Plan table function invocation with table arguments#5#7
Conversation
Changes adapted from trino/PR#14175 Original commit: 5c125b5ef0e355b7f89d4927171dc7dd029d0b18 Author: kasiafi Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
Changes adapted from trino/PR#14175 Original commit: a6f537d5519e34a4a46a411e6967d585b382c56f Author: kasiafi Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
Changes adapted from trino/PR#14175 Original commit: 4666472b0188aa26087840cdb587cc6e4495edef Author: kasiafi Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
Changes adapted from trino/PR#14175 Original commit: 1aea489884346822c812b1a242acc286e3e1248e Author: kasiafi Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
Changes adapted from trino/PR#14175 Original commit: 80c7fa0519eea07d8417d23908e8d1f8774dc3cd Author: kasiafi Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
4e4d17f to
f98676b
Compare
|
Finished it up to a point where everything is compiling. However I don't think its fully correct. Two things to look out for when running the tests.
|
Changes adapted from trino/PR#14175 Original commit: 8bd17171a8469b9351e2fd7d9f2f49f4af9ea209 Author: kasiafi Modifications were made to adapt to Presto including: Rewritting the UnaliasSymbolReferences off of Unnest Example Add a static coerce function passing in required values based off of current coerce function Co-authored-by: kasiafi <30203062+kasiafi@users.noreply.github.com>
|
Late night fix. I think I got the coerce thing handled. Made a static method and passed in all of the required objects from RelationPlanner. So just number 1 now. |
|
Investigating current issue With a breakpoint at The first value When stepping into get() , TranslateNameIntoSymbols correctly outputs c1 as a field. However both expressionsToVariables and expressionsToExpressions are not filled in. When running on |
|
After discussions with Aditi, we need to fill out the Translator with the fields from sourceDescriptor.getAllFieldCount(). Attempted to put in via However fails with c1 Not analyzed. Need to figure out where the analyzed c1 is and place it in the Translator. Update:
|
|
Fixed some issues in debugging the tests. Now the failure of the test is a plan mismatch. The expected is The actual is |
|
PlanMatchingVisitor investigation. When we reach the plan node for the tableFunctionNode ID7 we do find some matches. PlanMatching State Number 2 looks like the correct one. When looping through state number 2, It then does not find a sources match. As a result it exits here. Breakpoint used for debugging In trino with default test the equivalent is this is the successful path in Trino In presto we hit part of the successful path States is empty at this point States in trino at this point Error was due to not including |
|
Equivalent visitPlan Presto: Trino: Trino states: Presto states: Difference in matchSources Expected matching PlanNode source from Trino, Presto's At the point of hitting TableFunctionNode, we are still okay. Patterns continue without tableFunctionNode In Trino, At this point we are brought down to values nodes. |
|
Full plan output On Node#6 we get We expect to see Node 2 take away On Node 2 we see the two possibilities here Values Node fails to match: No States |
|
Trino Value Matcher Presto Value Matcher Breakpoint at visitPlan: This is failing at visitPlan Matcher 1 is true but matcher 2 is false. The number of output variables from the values node does not match the expected number. We expect 1 but our node is 0. For some reason in presto we expect 1 but in trino we don't expect any. |
|
@mohsaka Thanks for the analysis. The mismatch in ValueMatcher is due to an incorrect setting for the expected value in the test. There is another mismatch in TableFunctionMatcher. In Trino partitionBy and orderings are Symbols and only name is compared, while in Presto they are VariableReferenceExpression and both name and type are compared, and getExpectedValue will set the type to Unknown that is different from the actual type. I've fixed these issues in the latest commit by adding the value() function and ignoring the type comparison in VariableReferenceExpression. |
Description
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
If release note is NOT required, use: