[GLUTEN-10134][VL] (STORE_ASSIGNMENT_POLICY defaults to ANSI) Preserve store assignment cast modes - #12051
[GLUTEN-10134][VL] (STORE_ASSIGNMENT_POLICY defaults to ANSI) Preserve store assignment cast modes#12051ReemaAlzaid wants to merge 28 commits into
Conversation
|
Run Gluten Clickhouse CI on x86 |
|
Could you please review this? @rui-mo |
rui-mo
left a comment
There was a problem hiding this comment.
Do we have any background to support this policy, and would it make sense to add some tests covering the configurable behaviors under different policy settings?
| type == BIGINT(); | ||
| } | ||
|
|
||
| bool isAnsiSupported(const TypePtr& fromType, const TypePtr& toType) { |
There was a problem hiding this comment.
There's also a check on the Velox side. Will we maintain the ANSI support check only here?
There was a problem hiding this comment.
This is intended to mirror the existing Velox ANSI support side check not to replace it, since that check is private today, I added this local helper for the expression level ANSI/legacy cast mode. I’ll add a comment to make sure we keep it aligned with Velox
| } | ||
| } | ||
|
|
||
| testGluten("storeAssignmentPolicy default ANSI is independent from ANSI mode") { |
There was a problem hiding this comment.
This doesn't appear to be a variant of a Spark test. Can we move it to a new suite under backends-velox/?
|
|
||
| spark.sql("CREATE TABLE store_assignment_ansi (c INT) USING PARQUET") | ||
| intercept[Exception] { | ||
| spark.sql("INSERT INTO store_assignment_ansi SELECT '2147483648'").collect() |
There was a problem hiding this comment.
Can we add a check for the exception message to confirm the expected exception is thrown?
My understanding is that Spark applies |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
philo-he
left a comment
There was a problem hiding this comment.
Thanks for the PR. Two comments, possibly due to missing context on my side.
| std::make_shared<SparkCastHooks>(config, allowOverflow)); | ||
| } | ||
|
|
||
| class SparkAnsiCastCallToSpecialForm : public exec::CastCallToSpecialForm { |
There was a problem hiding this comment.
I am still wondering if we can move or keep these code on Velox side, and just call the register API in Gluten C++ code. Could you please clarify?
There was a problem hiding this comment.
Yes. Existing Velox Spark cast depends on session ansiEnabled, but Gluten needs Substrait’s per expression cast mode. This patch adds thin Gluten registration while reusing Velox cast code. I agree we can move it to Velox and call the registration API from Gluten
| // Keep this in sync with Velox's SparkCastCallToSpecialForm::isAnsiSupported. | ||
| // Velox's helper is private today; this local copy is needed for expression-level | ||
| // ANSI and legacy cast modes. | ||
| bool isAnsiSupported(const TypePtr& fromType, const TypePtr& toType) { |
There was a problem hiding this comment.
If we really need it, to reduce code maintenance effort, could we make it public in Velox to allow calling it here?
There was a problem hiding this comment.
Created the pr here facebookincubator/velox#17687
|
Run Gluten Clickhouse CI on x86 |
…om/ReemaAlzaid/incubator-gluten into store-assignment-policy-cast-modes
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
|
@ReemaAlzaid, thanks for the update. Could you check if the CI failures are related? |
|
The pr got merged facebookincubator/velox#17687 @philo-he |
@ReemaAlzaid, thanks for the update. I noticed the CI failed. Is it because Gluten's referenced Velox branch doesn't yet include that Velox commit? If so, you could reference the PR ID in the following code so Gluten picks up that Velox patch for the CI tests. gluten/ep/build-velox/src/get-velox.sh Line 28 in cd99450 |
|
Run Gluten Clickhouse CI on x86 |
…om/ReemaAlzaid/incubator-gluten into store-assignment-policy-cast-modes
|
Run Gluten Clickhouse CI on x86 |
|
Hey @philo-he, the velox commit is added now and has been merged I just forgot to update the ansi config key class here Fix Spark ANSI config key in cast tests. So the CI should run fine now |
|
@ReemaAlzaid, thanks for the update. It seems that CI failure is related. Could you check further? Thank you! |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
1 similar comment
|
Run Gluten Clickhouse CI on x86 |
10c7de5 to
7dc2bef
Compare
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
@ReemaAlzaid, thanks for the iterating. Could you check if the clickhouse backend CI failure is related? You can use this account to login: gluten/docs/get-started/ClickHouse.md Line 651 in 82da039 If not related, you can re-trigger that CI by posting a comment to this PR page: |
| toType: String)(f: => Unit): Unit = { | ||
| val exception = intercept[AnalysisException](f) | ||
| // Older Spark versions report the types in lower case, e.g. "string to int", | ||
| // while newer ones quote them in upper case, e.g. "STRING" to "INT". |
There was a problem hiding this comment.
Nit:
Could you leave some details about since which Spark versions upper case is used? Then, we can remove the case conversion when those old Spark versions are deprecated.
You may clarify like this "Since Spark xxx, the types in the exception message are in upper case, e.g. ...."
There was a problem hiding this comment.
Nit: Could you leave some details about since which Spark versions upper case is used? Then, we can remove the case conversion when those old Spark versions are deprecated.
You may clarify like this "Since Spark xxx, the types in the exception message are in upper case, e.g. ...."
Sure I have added the comment thanks
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
Hey @philo-he thanks for the review! I have addressed the comments and reran the CI |
What changes are proposed in this pull request?
This PR preserves Spark cast eval mode when Gluten translates casts to Velox, so store assignment casts keep ANSI behavior even when
spark.sql.ansi.enabled=false.#10134
TRY_CASTas Velox try-cast.STORE_ASSIGNMENT_POLICY=ANSI.How was this patch tested?
Was this patch authored or co-authored using generative AI tooling?