mihailoale-db commented on code in PR #59591:
URL: https://github.com/apache/spark/pull/59591#discussion_r4313653593


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/resolver/AsOfJoinResolver.scala:
##########
@@ -73,8 +73,15 @@ class AsOfJoinResolver(
       Seq(leftNameScope.output, rightNameScope.output)
     )
 
+    // MATCH_CONDITION is resolved a second time below. Keep the hidden column 
IDs here too, so a

Review Comment:
   nit: Let's move to Scala doc instead of having this inline comment, seems 
nicer.



##########
sql/core/src/test/resources/sql-tests/inputs/join-asof-semantics.sql:
##########
@@ -321,3 +321,41 @@ FROM using_left l LEFT ASOF JOIN (using_right r LEFT JOIN 
using_keys d USING (k)
 SELECT l.lts, d.k
 FROM using_left l LEFT ASOF JOIN (using_right r JOIN using_keys d USING (k))
   MATCH_CONDITION (l.k <= d.k);
+
+-- FVT-ASOF-2-043: an ASOF JOIN with USING passes up the hidden columns of its 
inputs

Review Comment:
   Let's address this:
   ```
     Finding 4: the right-side half of the fix is untested (Medium)
   
     Tests 046 and 047 only put the hidden column on the left. The 
rightNameScope.hiddenOutput term matters when the right side keeps the original 
ID, which happens with a first CTE reference. I traced this case myself:
   
     WITH c AS (SELECT * FROM using_keys)
     SELECT x.k, d.k FROM using_keys x
     ASOF JOIN (using_right r LEFT JOIN c d USING (k)) MATCH_CONDITION (x.k >= 
d.k);
   
     Before the PR, the left side's K→K_x overwrites the right side's K→K. The 
simpler variant with using_keys d doesn't need this term, because there d is 
the copy that gets a new ID.
   
     Fix: add the query above as a golden, and confirm it fails when only the 
right-side term is reverted.
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to