comphead commented on code in PR #6173: URL: https://github.com/apache/datafusion-comet/pull/6173#discussion_r4488344314
########## docs/source/contributor-guide/regex-fixtures.md: ########## @@ -0,0 +1,103 @@ +<!-- +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. +--> + +# Java regex parity fixtures + +The committed `spark/src/test/resources/regex/rlike-java-fixtures.json` records +Java `Pattern.compile(pattern).matcher(subject).find()` results for patterns +admitted by `CometRegex`. Rust's `test_rlike_java_fixtures` exercises the actual +`RLike` expression with scalar and UTF-8 array inputs against these answers. +It neither starts a JVM nor generates expected results during the test. The +crate's existing JNI build/link requirements still apply; see [Development](development.md). + +## Regenerating + +From the repository root, use JDK 17. First check the committed fixtures without +overwriting them: + +```shell +java dev/GenerateRegexFixtures.java /tmp/rlike-java-fixtures.json +cmp spark/src/test/resources/regex/rlike-java-fixtures.json /tmp/rlike-java-fixtures.json Review Comment: This `cmp` will report a difference on any JDK build other than the recorded `Eclipse Adoptium 17.0.20.1+1`, because lines 2 and 3 of the JSON hold `jdk_vendor` and `jdk_runtime_version`. I ran the generator from the PR head with JDK `17.0.19` (Apple build) and the only differing lines were those two, but `cmp` still exits non-zero. A contributor on another vendor or patch release would read that as a stale fixture file. Could the check skip the header, for example `diff <(sed 1,3d spark/src/test/resources/regex/rlike-java-fixtures.json) <(sed 1,3d /tmp/rlike-java-fixtures.json)`, and could the text say that the two header lines are expected to differ? ########## native/spark-expr/src/predicate_funcs/rlike.rs: ########## @@ -183,6 +183,76 @@ mod tests { use arrow::datatypes::{Field, Int32Type, Int8Type, UInt64Type}; use datafusion::physical_expr::expressions::{Column, Literal}; + #[test] + fn test_rlike_java_fixtures() { + #[derive(serde::Deserialize)] + struct Fixture { + category: String, + pattern: String, + subject: String, + expected: bool, + } + #[derive(serde::Deserialize)] + struct Fixtures { + cases: Vec<Fixture>, + } + let fixtures: Fixtures = serde_json::from_str(include_str!(concat!( + env!("CARGO_MANIFEST_DIR"), + "/../../spark/src/test/resources/regex/rlike-java-fixtures.json" + ))) + .expect("valid Java regex fixtures"); + assert!(!fixtures.cases.is_empty()); + for fixture in fixtures.cases { Review Comment: Every failure path in this loop panics, so a `regex` upgrade that changes results reports only the first affected pattern and the developer has to fix, rerun and repeat to see the rest. For an upgrade the useful output is the whole set of affected `(pattern, subject)` pairs. Could the loop push each `context` string into a `Vec<String>` and assert it is empty after the loop, printing the count and the first N entries? The compile error case can follow the same route by matching on `RLike::try_new` instead of unwrapping. -- 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]
