andygrove commented on code in PR #5037: URL: https://github.com/apache/datafusion-comet/pull/5037#discussion_r3661101995
########## 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: Added the pair in f4cdc33e9. `encode_unmappable.sql` carries `MaxSparkVersion: 3.5` and asserts substitution over both column and literal input, pinning the bytes rather than only asserting Spark/Comet agreement: `encode('café', 'US-ASCII') = CAST('caf?' AS BINARY)`, plus CJK input under both US-ASCII and ISO-8859-1 (and the ISO-8859-1 case where `é` maps to 0xE9 but the CJK characters do not). `encode_unmappable_strict.sql` carries `MinSparkVersion: 4.0` and uses `expect_error(MALFORMED_CHARACTER_CODING)` for literal US-ASCII, literal ISO-8859-1 and column input, with a sentinel `query` over ASCII-only rows first so the assertions cannot be satisfied by an operator-level fallback. ########## 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: Added all three branches in f4cdc33e9. - `encode_invalid_charset_strict.sql` (`MinSparkVersion: 4.0`): `expect_error(INVALID_PARAMETER_VALUE)` for `windows-1252` as a literal, `windows-1252` as a column value, and a name the JVM does not know at all, since `CharsetProvider.forName` rejects all three the same way. - `encode_invalid_charset.sql` (`MaxSparkVersion: 3.5`): `windows-1252` succeeds because 3.x calls `String.getBytes` directly, and an unknown name gives `expect_error(UnsupportedEncodingException)`. - `encode_legacy_charsets.sql` (`MinSparkVersion: 4.0`, `spark.sql.legacy.javaCharsets=true`): `windows-1252` and `Shift_JIS` both succeed, demonstrating the flag rather than only asserting it is carried. Rows are chosen to be representable in windows-1252 so the still-strict coding-error action does not fire and the two flags stay independently tested. Each of the error fixtures leads with a sentinel `query` over valid input. `CometSqlFileTestSuite` enforces that convention and also disables constant folding, so the literal-input error cases really do run through the dispatcher. ########## 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: Restored UTF-16LE and added UTF-32 in f4cdc33e9, with a comment recording why UTF-16LE is not redundant with UTF-16 (BOM) or UTF-16BE (byte order). All seven `VALID_CHARSETS` entries now have coverage. The stale "six charsets" wording in the description is 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]
