LukaZdravic commented on PR #59016:
URL: https://github.com/apache/spark/pull/59016#issuecomment-5909427048
> Mostly looks good, leaving a few comments below:
>
> 1. Nested empty structs still hit the CreateStruct NULL rewrite (related,
not new logic)
>
> STRUCT<e: STRUCT<>> is newly accepted and is decomposed.
alignOperandsForComparison rebuilds it as CreateStruct(GetStructField(…)), so a
NULL operand becomes {e: NULL} and NULL >= NULL matches. Top-level STRUCT<>
avoids that on purpose; the nested form does not.
>
> The new NULL test only uses CAST(NULL AS STRUCT<>). Please make sure
#59134 covers CAST(NULL AS STRUCT<e: STRUCT<>>) as well. I would not block this
PR on that if #59134 is the agreed follow-up.
>
> 2. Golden FVT-ASOF-4-018b does not test what the comment says
>
> The comment is “elements tie, so length decides”, but the right side is
only length 1 and 3. Left length 2 with >= matches only the shorter array. That
does not show length as a tie-break among several matches.
>
> The Scala test already has the right shape (lengths 1, 2, and 3, all four
operators). Mirror that in join-asof-datatypes.sql.
>
> 3. The mixed-type ARRAY fix has no analyzer unit test
>
> AsOfJoinSortMergeSQLSuite is a good regression test, but the original
failure was analysis-time CreateArray type-checking. ResolveAsOfJoinSuite
already pins empty-array ZipWith; a sibling case for ARRAY<STRUCT<f: INT, ts:
TIMESTAMP>> asserting the lambda body is CreateNamedStruct (not CreateArray)
would lock the fix cheaply.
Thank you! Addressed 2 and 3: FVT-ASOF-4-018b now has lengths 1, 2 and 3
with all four operators (4-018b to 4-018e), and ResolveAsOfJoinSuite checks
that a mixed-type ARRAY<STRUCT> gets a CreateNamedStruct element distance. For
1, #59134 will add CAST(NULL AS STRUCT<e: STRUCT<>>) once this PR merges, since
master rejects that type until then.
--
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]
