LuciferYang commented on PR #12888:
URL: https://github.com/apache/gluten/pull/12888#issuecomment-5409278527
Thanks, five rounds of this got the comments into decent shape. What I took
and what I did not:
**Taken.** The version stamp is gone from the `Stop task set if
FileAlreadyExistsException was thrown` comment: the same text sits in the 4.0
and 4.1 settings where no such run was done, so naming 3.4.4 there was
misleading. The reproduction detail lives in the PR description instead. `Velox
computes correctly; only the collect path fails` is gone too, from all four
modules rather than just the two this patch adds, since this patch is what
would have spread it from one file to three; the comment now stops at the
mechanism the repo can show. The duplicate copy of that explanation inside the
test body is now a pointer. The rewrite-pair comment leads with the reason for
the exclusion and names the path, so the settings file reads on its own:
`Excluded in favour of the GlutenCastSuite rewrite, which drops the
Long.MinValue assertion: collect() -> toJavaTimestamp ->
rebaseGregorianToJulianMicros overflows.` And the ClickHouse comment no longer
depends on where the vanilla exclusion sit
s in the block.
**Not taken, with reasons.**
Re-adding the nuance that the failure is in the harness rather than in the
Velox cast: that is the claim an earlier comment in this same review series
asked me to remove, and rightly. The comment already names `toJavaTimestamp ->
rebaseGregorianToJulianMicros`, which is the harness path. What is missing is
the conclusion "so the Velox result is correct", and nothing in the tree
supports it, least of all this rewrite, which is defined by not asserting
`Long.MinValue`. Establishing it needs an assertion that checks the cast result
without going through `collect()`, and I did not find a helper for that.
Extracting the rewrite body into a shared helper: per-module duplication is
what this repo does for rewrites. `gluten-ut/common` contains no `testGluten`
bodies at all, and the closest precedent, `testGluten("cast string to
timestamp")`, is copied into twelve wrapper files across the five modules. The
four wrappers also extend differently named parents per Spark version.
Consolidating them is worth doing, but as its own change rather than inside a
comment cleanup.
One correction on a stale comment: `Reproduced on Spark 3.4.4` was flagged
again at `gluten-ut/spark40/.../VeloxTestSettings.scala:870` seventeen seconds
after I had pushed its removal, so that one was already fixed.
--
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]
