You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "YutaLin (via GitHub)" <gi...@apache.org> on 2026/05/13 20:11:22 UTC
[PR] feat: Support Spark Expression Encode [datafusion-comet]
YutaLin opened a new pull request, #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315
## Which issue does this PR close?
Closes #3183
## Rationale for this change
Support expression Encode
## What changes are included in this PR?
* Add StringEncode in string serde
* Update shims in spark3.4/3.5/4.0/4.1/4.2 to catch Encode
## How are these changes tested?
Add encode.sql and run it in spark 3.4/3.5/4.0
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#issuecomment-4626703547
Sure. Yeah have been working on some doc changes causing merge conflicts. I will review it shortly
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "YutaLin (via GitHub)" <gi...@apache.org>.
YutaLin commented on PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#issuecomment-4607566835
Hi @andygrove,
thanks for the review! I've fix the conflict!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#issuecomment-4625071829
Seems like there are still some more conflicts @YutaLin .
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "YutaLin (via GitHub)" <gi...@apache.org>.
YutaLin commented on code in PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#discussion_r3263536000
##########
spark/src/main/scala/org/apache/comet/serde/strings.scala:
##########
@@ -495,4 +495,33 @@ trait CommonStringExprs {
None
}
}
+
+ def stringEncode(
+ expr: Expression,
+ charset: Expression,
+ value: Expression,
+ inputs: Seq[Attribute],
+ binding: Boolean): Option[Expr] = {
+ charset match {
+ case Literal(str, DataTypes.StringType)
+ if str != null && str.toString.toLowerCase(Locale.ROOT) == "utf-8" =>
Review Comment:
The StandardCharsets.UTF_8.equals(Charset.forName(str)) would include "utf-8", "UTF-8", "utf8" and "UTF8", but last two one are not supported in Spark4.0. I'd suggest we keep current comparison.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#discussion_r3237198246
##########
spark/src/main/scala/org/apache/comet/serde/strings.scala:
##########
@@ -495,4 +495,33 @@ trait CommonStringExprs {
None
}
}
+
+ def stringEncode(
+ expr: Expression,
+ charset: Expression,
+ value: Expression,
+ inputs: Seq[Attribute],
+ binding: Boolean): Option[Expr] = {
+ charset match {
+ case Literal(str, DataTypes.StringType)
+ if str.toString.toLowerCase(Locale.ROOT) == "utf-8" =>
Review Comment:
Is it possible for `str` to be null? If so, it could cause NPE here
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "YutaLin (via GitHub)" <gi...@apache.org>.
YutaLin commented on code in PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#discussion_r3263523419
##########
spark/src/test/resources/sql-tests/expressions/string/encode.sql:
##########
@@ -0,0 +1,61 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- Tests for the SQL `encode(str, charset)` function.
+--
+-- Spark 3.x: Encode is a BinaryExpression(value, charset).
+-- Spark 4.x+: Encode is RuntimeReplaceable; the analyzer rewrites it to
+-- StaticInvoke(classOf[Encode], BinaryType, "encode", ...)
+
+statement
+CREATE TABLE test_encode_utf8(s string) USING parquet
+
+statement
+INSERT INTO test_encode_utf8 VALUES ('hello'), ('world'), (''), ('café'), (NULL)
+
+query
+SELECT encode(s, 'utf-8') FROM test_encode_utf8
Review Comment:
uft8/UTF8 only supports version before v4.0
https://github.com/apache/datafusion-comet/pull/4315#issuecomment-4445627791
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "YutaLin (via GitHub)" <gi...@apache.org>.
YutaLin commented on PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#issuecomment-4513286477
Hi @coderfender
Could you help me check this again? Thanks!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#issuecomment-4445003819
Thanks @YutaLin. LGTM overall. Could you address feedback, then I'll kick off CI
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "YutaLin (via GitHub)" <gi...@apache.org>.
YutaLin commented on PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#issuecomment-4445627791
Hi @andygrove, thanks for the review!
I've extract encode method and add null check.
About "Spark accepts utf8 as an alias for UTF-8", spark only supports alias before 3.5, because it uses JDK `Charset.forName`. After 4.0, it has a whitelist check, so it doesn't support alias. I'd suggest we keep only utf-8 now, WDYT?
https://spark.apache.org/docs/4.0.0/sql-migration-guide.html#upgrading-from-spark-sql-35-to-40
> Since Spark 4.0, the encode() and decode() functions support only the following charsets ‘US-ASCII’, ‘ISO-8859-1’, ‘UTF-8’, ‘UTF-16BE’, ‘UTF-16LE’, ‘UTF-16’, ‘UTF-32’. To restore the previous behavior when the function accepts charsets of the current JDK used by Spark, set spark.sql.legacy.javaCharsets to true.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "YutaLin (via GitHub)" <gi...@apache.org>.
YutaLin commented on code in PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#discussion_r3263653877
##########
spark/src/main/spark-4.x/org/apache/comet/shims/ShimCometExprs.scala:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.shims
+
+import org.apache.spark.sql.catalyst.expressions._
+import org.apache.spark.sql.catalyst.expressions.objects.StaticInvoke
+import org.apache.spark.sql.internal.types.StringTypeWithCollation
+import org.apache.spark.sql.types.{BinaryType, BooleanType}
+
+import org.apache.comet.serde.CommonStringExprs
+import org.apache.comet.serde.ExprOuterClass.Expr
+
+trait ShimCometExprs extends CommonStringExprs {
Review Comment:
I've rename it `CometExprShimCommon`.
##########
spark/src/main/spark-4.1/org/apache/comet/shims/CometExprShim.scala:
##########
@@ -92,6 +92,19 @@ trait CometExprShim extends CommonStringExprs {
val Seq(bin, charset, _, _) = s.arguments
stringDecode(expr, charset, bin, inputs, binding)
+ case s: StaticInvoke
+ if s.staticObject == classOf[Encode] &&
+ s.dataType.isInstanceOf[BinaryType] &&
+ s.functionName == "encode" &&
+ s.arguments.size == 4 &&
+ s.inputTypes == Seq(
+ StringTypeWithCollation(supportsTrimCollation = true),
+ StringTypeWithCollation(supportsTrimCollation = true),
+ BooleanType,
+ BooleanType) =>
+ val Seq(value, charset, _, _) = s.arguments
+ stringEncode(expr, charset, value, inputs, binding)
+
Review Comment:
Yes, thanks for reminder!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "YutaLin (via GitHub)" <gi...@apache.org>.
YutaLin commented on PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#issuecomment-4626694294
Hi @coderfender
Thanks for pointing that out. The previous conflict had already been resolved and reviewed. Since the PR wasn't merged immediately afterward, additional changes on the main branch introduced a new conflict. I've now merged the latest main branch and resolved that conflict as well.
Could you please take another look when you have a chance?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#issuecomment-4606094139
Thanks @YutaLin. This looks good. Could you fix the conflicts?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on code in PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#discussion_r3253731787
##########
spark/src/main/scala/org/apache/comet/serde/strings.scala:
##########
@@ -495,4 +495,33 @@ trait CommonStringExprs {
None
}
}
+
+ def stringEncode(
+ expr: Expression,
+ charset: Expression,
+ value: Expression,
+ inputs: Seq[Attribute],
+ binding: Boolean): Option[Expr] = {
+ charset match {
+ case Literal(str, DataTypes.StringType)
+ if str != null && str.toString.toLowerCase(Locale.ROOT) == "utf-8" =>
Review Comment:
This seems like a fragile check . Perhaps a better check would be to use `StandardCharsets` ?
##########
spark/src/test/resources/sql-tests/expressions/string/encode.sql:
##########
@@ -0,0 +1,61 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- Tests for the SQL `encode(str, charset)` function.
+--
+-- Spark 3.x: Encode is a BinaryExpression(value, charset).
+-- Spark 4.x+: Encode is RuntimeReplaceable; the analyzer rewrites it to
+-- StaticInvoke(classOf[Encode], BinaryType, "encode", ...)
+
+statement
+CREATE TABLE test_encode_utf8(s string) USING parquet
+
+statement
+INSERT INTO test_encode_utf8 VALUES ('hello'), ('world'), (''), ('café'), (NULL)
+
+query
+SELECT encode(s, 'utf-8') FROM test_encode_utf8
Review Comment:
I believe spark accepts any variant of `utf-8` (`utf8 /UTF8` etc) . May be a good idea to add those tests ?
##########
spark/src/main/spark-4.1/org/apache/comet/shims/CometExprShim.scala:
##########
@@ -92,6 +92,19 @@ trait CometExprShim extends CommonStringExprs {
val Seq(bin, charset, _, _) = s.arguments
stringDecode(expr, charset, bin, inputs, binding)
+ case s: StaticInvoke
+ if s.staticObject == classOf[Encode] &&
+ s.dataType.isInstanceOf[BinaryType] &&
+ s.functionName == "encode" &&
+ s.arguments.size == 4 &&
+ s.inputTypes == Seq(
+ StringTypeWithCollation(supportsTrimCollation = true),
+ StringTypeWithCollation(supportsTrimCollation = true),
+ BooleanType,
+ BooleanType) =>
+ val Seq(value, charset, _, _) = s.arguments
+ stringEncode(expr, charset, value, inputs, binding)
+
Review Comment:
Seems like we missed removing this piece ?
##########
spark/src/main/spark-4.x/org/apache/comet/shims/ShimCometExprs.scala:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.shims
+
+import org.apache.spark.sql.catalyst.expressions._
+import org.apache.spark.sql.catalyst.expressions.objects.StaticInvoke
+import org.apache.spark.sql.internal.types.StringTypeWithCollation
+import org.apache.spark.sql.types.{BinaryType, BooleanType}
+
+import org.apache.comet.serde.CommonStringExprs
+import org.apache.comet.serde.ExprOuterClass.Expr
+
+trait ShimCometExprs extends CommonStringExprs {
Review Comment:
seems like the trait name seems to be different from `Comet<>` ?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#issuecomment-6199985905
Hi @YutaLin Would you mind resolving the conflicts please?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "dwsmith1983 (via GitHub)" <gi...@apache.org>.
dwsmith1983 commented on PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#issuecomment-5976390625
#3183 was closed by #5037. `encode` now goes through the codegen dispatcher on every profile (3.x via #5037, 4.x via the `StaticInvoke` catch-all from #5692), so it matches Spark on malformed UTF-8 too, which the cast lowering here doesn't. If the native cast is still worth it as a speedup, it needs a rebase and numbers against the dispatcher; otherwise I think this can close.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "coderfender (via GitHub)" <gi...@apache.org>.
coderfender commented on PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#issuecomment-4513364514
Sorry for the delay. Let me look into it shortly @YutaLin
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "YutaLin (via GitHub)" <gi...@apache.org>.
YutaLin commented on PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#issuecomment-4484386075
Hi @coderfender
Thanks for the review, i've made the change, please help me review again!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#discussion_r3237243230
##########
spark/src/main/scala/org/apache/comet/serde/strings.scala:
##########
@@ -495,4 +495,33 @@ trait CommonStringExprs {
None
}
}
+
+ def stringEncode(
+ expr: Expression,
+ charset: Expression,
+ value: Expression,
+ inputs: Seq[Attribute],
+ binding: Boolean): Option[Expr] = {
+ charset match {
+ case Literal(str, DataTypes.StringType)
+ if str.toString.toLowerCase(Locale.ROOT) == "utf-8" =>
Review Comment:
My AI reviewer said "Spark accepts utf8 as an alias for UTF-8". Could you check if this is accurate? We may need to handle that case as well.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#discussion_r3237192893
##########
spark/src/main/spark-4.0/org/apache/comet/shims/CometExprShim.scala:
##########
@@ -93,6 +93,19 @@ trait CometExprShim extends CommonStringExprs {
val Seq(bin, charset, _, _) = s.arguments
stringDecode(expr, charset, bin, inputs, binding)
+ case s: StaticInvoke
+ if s.staticObject == classOf[Encode] &&
+ s.dataType.isInstanceOf[BinaryType] &&
+ s.functionName == "encode" &&
+ s.arguments.size == 4 &&
+ s.inputTypes == Seq(
+ StringTypeWithCollation(supportsTrimCollation = true),
+ StringTypeWithCollation(supportsTrimCollation = true),
+ BooleanType,
+ BooleanType) =>
+ val Seq(value, charset, _, _) = s.arguments
+ stringEncode(expr, charset, value, inputs, binding)
Review Comment:
This looks identical for all Spark 4.x versions - could this move to the spark-4.x shim to avoid duplicating?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "YutaLin (via GitHub)" <gi...@apache.org>.
YutaLin commented on PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#issuecomment-5065868681
Hi @andygrove
Thanks for reviewing this. I kept the cast lowering for valid UTF-8, documented the malformed UTF-8 limitation next to the implementation and in the compatibility guide, and captured the reproduction under #4764. I also loosened the 4.x StaticInvoke match and fixed the trailing newline.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Support Spark Expression Encode [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4315:
URL: https://github.com/apache/datafusion-comet/pull/4315#issuecomment-5441800402
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled.
Lowering `encode(str, 'utf-8')` to a `CAST(string AS binary)` is a neat way to get this for free, and the `CometExprShimCommon` trait shared across the 4.x shims is the right structure for the `StaticInvoke` rewrite. The SQL fixture covers empty strings, NULL, multibyte, and the mixed-case charset literal, which is good.
Three things.
**The malformed-UTF-8 divergence is not reported to users**
Spark replaces malformed bytes during `encode`, while the cast lowering preserves them. That is a wrong answer, not a fallback, and right now the only record of it is an `ignore(...)` line in `encode.sql` plus a code comment.
`encode` should report `Incompatible(Some(...))` for this, or at minimum the divergence needs to appear on the string compatibility page so a user can find it. As it stands, `expressions.md` will show `encode` as supported with no caveat and a user with dirty string data gets silently different bytes. That is the exact case Comet's `Incompatible` mechanism exists for.
If you would rather not gate it, that is a discussion worth having explicitly, but it should not be an implicit consequence of where the code happens to live.
**Charset alias matching**
```scala
str.toString.toLowerCase(Locale.ROOT) == "utf-8"
```
Java's `Charset.forName` accepts `UTF8`, `utf8`, and `unicode-1-1-utf-8` as aliases for UTF-8, and Spark accepts whatever `Charset.forName` accepts. So `encode(s, 'UTF8')` falls back here even though it is exactly the case this PR handles.
Would `Try(Charset.forName(name).name() == "UTF-8").getOrElse(false)` be better? It handles aliases, and an invalid charset name naturally falls back to Spark, which then raises the proper `INVALID_PARAMETER_VALUE`.
**Duplication between the 3.4 and 3.5 shims**
The additions to `spark-3.4/CometExprShim.scala` and `spark-3.5/CometExprShim.scala` are identical. The 4.x side already shares through `CometExprShimCommon`. Is there a `spark-3.x` shared location that could host the same dispatch for 3.4 and 3.5? Two copies of a three-line match is minor now, but it is the pattern that makes per-version gaps show up later, and CI only lints a subset of the version matrix.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org