mbutrovich commented on code in PR #5037: URL: https://github.com/apache/datafusion-comet/pull/5037#discussion_r3658420444
########## spark/src/test/resources/sql-tests/expressions/string/encode.sql: ########## @@ -0,0 +1,84 @@ +-- 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 (StringType, StringType) -> BinaryType. +-- +-- `encode` runs through the codegen dispatcher (Spark's own doGenCode inside the Comet +-- pipeline) so behavior matches Spark exactly across all supported charsets and across the +-- Spark 4.0 `legacyCharsets` / `legacyErrorAction` modes. This is the dual of the `decode` +-- codegen-dispatch path (#4465). +-- +-- Each `query` block runs checkSparkAnswerAndOperator, which fails if the expression fell back +-- to Spark instead of executing natively, so these are non-vacuous. +-- Config: spark.comet.exec.scalaUDF.codegen.enabled=true + +statement +CREATE TABLE test_encode(s string) USING parquet + +statement +INSERT INTO test_encode VALUES ('hello'), ('world'), (''), ('café'), (NULL) + +-- Charset form over multiple charsets + +query +SELECT encode(s, 'utf-8') FROM test_encode + +query +SELECT encode(s, 'UTF-8') FROM test_encode + +query +SELECT encode(s, 'UTF-16') FROM test_encode + +query +SELECT encode(s, 'UTF-16BE') FROM test_encode + +query +SELECT encode(s, 'ISO-8859-1') FROM test_encode + +-- US-ASCII: use ASCII-only input. Non-ASCII input under US-ASCII throws on Spark 4.0+ strict +-- mode (matching Spark), so it is not exercised here. Review Comment: The comment works out the per-version split correctly and then declines to test it. The split is real: on 3.4 and 3.5, `nullSafeEval` is `input1.toString.getBytes(toCharset)` (Spark `stringExpressions.scala:2708`), which substitutes 0x3F for unmappable characters, while on 4.0 and later `Encode.encode` builds an encoder via `CharsetProvider.newEncoder` with `CodingErrorAction.REPORT` and converts `CharacterCodingException` into `QueryExecutionErrors.malformedCharacterCoding("encode", charset)`, error class `MALFORMED_CHARACTER_CODING` (`stringExpressions.scala:3192-3200`, `CharsetProvider.scala:52-67`, `QueryExecutionErrors.scala:2844-2850`). The `decode` work this PR mirrors does cover its equivalent, with two version-split fixtures: `decode_invalid_utf8.sql` and `decode_invalid_utf8_strict.sql` (MinSparkVersion 4.0, `expect_error(MALFORMED_CHARACTER_CODING)`, plus a sentinel query so the assertion cannot be satisfied by an operator-level fallback). Please add the matching pair, an `encode_unmappable.sql` with MaxSparkVersion 3.5 asserting substitution and an `encode_unmappable_strict.sql` with MinSparkVersion 4.0 asserting the error. Right now the substituting branch and the throwing branch are both untested on every profile. ########## spark/src/test/resources/sql-tests/expressions/string/encode.sql: ########## @@ -0,0 +1,84 @@ +-- 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 (StringType, StringType) -> BinaryType. +-- +-- `encode` runs through the codegen dispatcher (Spark's own doGenCode inside the Comet +-- pipeline) so behavior matches Spark exactly across all supported charsets and across the +-- Spark 4.0 `legacyCharsets` / `legacyErrorAction` modes. This is the dual of the `decode` +-- codegen-dispatch path (#4465). +-- +-- Each `query` block runs checkSparkAnswerAndOperator, which fails if the expression fell back +-- to Spark instead of executing natively, so these are non-vacuous. +-- Config: spark.comet.exec.scalaUDF.codegen.enabled=true + +statement +CREATE TABLE test_encode(s string) USING parquet + +statement +INSERT INTO test_encode VALUES ('hello'), ('world'), (''), ('café'), (NULL) + +-- Charset form over multiple charsets + +query +SELECT encode(s, 'utf-8') FROM test_encode + +query +SELECT encode(s, 'UTF-8') FROM test_encode + +query +SELECT encode(s, 'UTF-16') FROM test_encode + +query +SELECT encode(s, 'UTF-16BE') FROM test_encode + +query +SELECT encode(s, 'ISO-8859-1') FROM test_encode Review Comment: The description says six charsets including UTF-16LE. This block plus line 62 covers utf-8, UTF-8, UTF-16, UTF-16BE, ISO-8859-1 and US-ASCII. UTF-16LE is absent and worth adding for real: UTF-16 emits a BOM and UTF-16LE does not, so the byte output genuinely differs. UTF-32 is the remaining entry in VALID_CHARSETS with no coverage. ########## spark/src/test/resources/sql-tests/expressions/string/encode.sql: ########## @@ -0,0 +1,84 @@ +-- 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 (StringType, StringType) -> BinaryType. +-- +-- `encode` runs through the codegen dispatcher (Spark's own doGenCode inside the Comet +-- pipeline) so behavior matches Spark exactly across all supported charsets and across the +-- Spark 4.0 `legacyCharsets` / `legacyErrorAction` modes. This is the dual of the `decode` +-- codegen-dispatch path (#4465). +-- +-- Each `query` block runs checkSparkAnswerAndOperator, which fails if the expression fell back +-- to Spark instead of executing natively, so these are non-vacuous. +-- Config: spark.comet.exec.scalaUDF.codegen.enabled=true Review Comment: `CharsetProvider.forName` permits only us-ascii, iso-8859-1, utf-8, utf-16be, utf-16le, utf-16 and utf-32 unless `spark.sql.legacy.javaCharsets` is set, and otherwise raises `invalidCharsetError`, error class `INVALID_PARAMETER_VALUE.CHARSET` (`CharsetProvider.scala:29-49`, `QueryExecutionErrors.scala:2834-2842`); on 3.x an unknown charset surfaces as UnsupportedEncodingException through `Platform.throwException` (`stringExpressions.scala:2716-2717`). So `encode(s, 'windows-1252')` has three distinct behaviors across version and conf. The description asserts the flag is honored by construction, which is true, but nothing demonstrates it. Add an invalid-charset error case and one query under `spark.sql.legacy.javaCharsets=true`. ########## docs/source/user-guide/latest/expressions.md: ########## @@ -562,7 +562,7 @@ expression-level). The `outer` variants are wired but marked `Incompatible`; the | `contains` | ✅ | — | | | `decode` | ✅ | — | | | `elt` | ✅ | Codegen dispatch | | -| `encode` | 🔜 | — | Lowers to `StaticInvoke(encode)` (not allowlisted); falls back | +| `encode` | ✅ | — | | Review Comment: The Implementation cell stays `—`, which is the correct generated value on 4.x: `encode` is registered in the `FunctionRegistry` on 3.4, 3.5 and 4.0 (`FunctionRegistry.scala:523`, `:527`, `:546` respectively), but on 4.x it is RuntimeReplaceable so no serde is keyed on `classOf[Encode]` in that profile and `GenerateDocs` emits the placeholder. On 3.4 and 3.5 the same cell becomes `Codegen dispatch` because `CometEncode` is a `CometCodegenDispatch` (`GenerateDocs.scala:194`). `decode` already has this shape, so the committed value is consistent. Just regenerate with `dev/generate-release-docs.sh` rather than hand-editing the row. -- 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]
