SNOW-2019483: fix select SQL in dynamic table

[sfc-gh-yuwang]

1. Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

   Fixes SNOW-2019483

2. Fill out the following pre-review checklist:

   • I am adding a new automated test(s) to verify correctness of my new code
     • If this test skips Local Testing mode, I'm requesting review from @snowflakedb/local-testing
   • I am adding new logging messages
   • I am adding a new telemetry message
   • I am adding new credentials
   • I am adding a new dependency
   • If this is a new feature/behavior, I'm adding the Local Testing parity changes.
   • I acknowledge that I have ensured my changes to be thread-safe. Follow the link for more information: Thread-safe Developer Guidelines
   • If adding any arguments to public Snowpark APIs or creating new public Snowpark APIs, I acknowledge that I have ensured my changes include AST support. Follow the link for more information: AST Support Guidelines

3. Please describe how your code solves the related issue.

   When creating a dynamic table, if it is created from a table function, the select on that table function must specify the column name instead of a '*', please refer to the Jira for details

[sfc-gh-snowflakedb-snyk-sa]

🎉 Snyk checks have passed. No issues have been found so far.

✅ security/snyk check is complete. No issues have been found. (View Details)

✅ license/snyk check is complete. No issues have been found. (View Details)

[graphite-app]

There appears to be a logic issue in this loop. The current implementation overwrites plan_2_resolve in each iteration, which means only the result from the last child node will be preserved. If a table function is found in any child node except the last one, that result will be lost.

Consider modifying the loop to return immediately when a match is found:

for node in plan.children_plan_nodes:
    plan_2_resolve = self.find_table_function_in_sql_tree(node)
    if plan_2_resolve:
        return plan_2_resolve

This ensures that the first matching table function found in any child node will be properly returned and processed.

Suggested change

	plan_2_resolve = None
	for node in plan.children_plan_nodes:
	plan_2_resolve = self.find_table_function_in_sql_tree(node)
	if plan_2_resolve:
	plan_2_resolve = None
	for node in plan.children_plan_nodes:
	plan_2_resolve = self.find_table_function_in_sql_tree(node)
	if plan_2_resolve:
	break
	if plan_2_resolve:

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

The find_table_function_in_sql_tree method can return None, but the code doesn't check for this before trying to access child.queries. Add a null check to use the resolved child only if it's not None: 'child = copy.deepcopy(child); resolved_child = self.find_table_function_in_sql_tree(child); if resolved_child is not None: child = resolved_child'

Spotted by Diamond (based on CI logs)

Is this helpful? React 👍 or 👎 to let us know.

[sfc-gh-jrose]

Can you fill out the PR description? The changes in this PR don't seem trivial to understand.

[sfc-gh-aling]

is it possible there are multiple table function call in one single DF operation?
from the logic the newer one will overwrite the existing one

[sfc-gh-yuwang]

I have tested that you cannot do nested table function call, this piece of code is just meant to know if there are changes happened in deeper layer to decide whether re-resolve current plan, so overwrite is ok.

[sfc-gh-aling]

we're doing in-place update, is this on purpose? usually when we want to modify a plan, we deepcopy and update the new one

[sfc-gh-yuwang]

the deepcopy code is outside this function, I'll figure out a way to move deep copy inside

[sfc-gh-aling]

I guess this is to flatten "*" to all identifiers.
I have a dumb question, what does plan.snowflake_plan.quoted_identifiers contain and why we are only selecting elements after len(child_plan.quoted_identifiers)

[sfc-gh-aalam]

Try using BFS. In the past, we have discovered that using DFS for snowpark plans can generate MaxRecursionDepthExceeded errors

[sfc-gh-aalam]

please add comments here to explain what we are trying to extract from TableFunctionJoin node

[sfc-gh-aalam]

this part also needs some explanation. what are we trying to achieve here?

[sfc-gh-jdu]

maybe we should move this deepcopy into find_table_function_in_sql_tree

[sfc-gh-jdu]

add docstring or comment for this function

[sfc-gh-jdu]

add a comment explaining it

[sfc-gh-jdu]

Suggested change

	replace '*' with explicit identifier in the select of table function.
	replace '*' with explicit column identifiers in the select of table function.

[sfc-gh-jdu]

also add a comment that why we need to do this, and actually it's for udtf, but we are not able to differentiate between udtf and other table functions, so we have to do it for all table functions.

[sfc-gh-aalam]

is is possible for a Selebtable to have a snowflake_plan where source_plan is None. Can we make sure that is not the case here?

[sfc-gh-yuwang]

I think the case shall not happen here, @sfc-gh-jdu can you help me confirm?

[sfc-gh-jdu]

I'm not sure it's possible. But to be safe, can we exit the recursion if it's None?

[graphite-app]

There's a typo in the comment: tale function should be table function

Suggested change

	# this is meant to decide whether the plan is select from a tale function
	# this is meant to decide whether the plan is select from a table function

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

[sfc-gh-aalam]

pinged offline there are still a couple of issues with this implementation

[sfc-gh-aalam]

can we also make sure that the same node is not added twice in the queue? an example where this can happen is in a dimond join case.