andygrove opened a new issue, #6893: URL: https://github.com/apache/datafusion-comet/issues/6893
### What is the problem the feature request solves? #6776 adds a rule that protobuf fields, native function arguments, and native function names describe the behavior they select rather than the Spark version, that the version check lives in one place in Scala, and that comments cite the Spark JIRA. I audited main against it. The JVM/native boundary is clean apart from the two names #6891 already tracks, `Cast.is_spark4_plus` and `get_json_object_spark34`. The other proto fields and native arguments that vary with the Spark version are already named for their behavior, such as `wrap_second_millisecond_overflow`, `normalize_neg_zero`, `filter_var_by_pair_nulls`, `r2_constant_dependent_is_perfect_fit`, `BloomFilterAgg.version`, `return_null_struct_if_all_fields_missing`, `allow_type_promotion` and `allow_timestamp_ltz_to_ntz`. No JNI signature or native config key carries a Spark version, and every comparison on `SPARK_VERSION` in shared Scala code sits behind a named predicate (`isSpark35Plus` through `isSpark42Plus`, `RegrSparkVersions`, `ArraySetSupport.normalizesArgumentsInPlan`). I also checked each flag that is set from a minor-version predicate against the newest patch of each earlier line (3.5.9, 4.0.4, 4.1.3). None of SPARK-40876, SPARK-46488, SPARK-47447, SPARK-47547, SPARK-53535, SPARK-56663, SPARK-57298 or SPARK-57329 was backported to those patches, so the minor-version checks are correct today. The two changes that did ship in patch releases, SPARK-48719 and SPARK-55969, already go through `RegrSparkVersions`. What is left is comments that describe these flags by Spark version, some of them wrongly, and one behavior decided in four places. 1. The `Regr` flag comments contradict the predicates that set the flags. `RegrSparkVersions.slopeFiltersVarByPairNulls` (SPARK-48719) is true from 3.5.2 and 4.0.0, and `r2DegenerateCasesSwapped` (SPARK-55969) from 3.5.9, 4.0.3, 4.1.2 and 4.2.0. But `expr.proto` documents `filter_var_by_pair_nulls` as true on "Spark 3.5+" and false on "Spark 3.4", and says the `r2_constant_dependent_is_perfect_fit` swap is in "the Spark versions Comet builds against for 3.5 and later (3.5.9, 4.0.3+, 4.1, 4.2)". Neither comment cites the JIRA. In `regr.rs` the `Regr` struct docs are right, but the `RegrR2Accumulator` and `RegrLineAccumulator` field docs, the branch comments in `RegrLineAccumulator::update_batch`, and the test comments say "Spark 3.5+" and "Spark 3.4". On 3.5.0, 3.5.1 or 4.1.1 they describe the wrong behavior. 2. `spark_bloom_filter.rs` cites the V2 format as "SPARK-XXXXX". It is SPARK-47547, which the Scala serde already cites. The `BloomFilterAgg.version` comments in `expr.proto` and `bloom_filter_agg.rs` don't cite it at all. 3. The `allow_type_promotion` comments in `operator.proto` and `schema_adapter.rs` say Spark 3.x rejects the widenings without citing SPARK-40876. The `SparkParquetOptions.allow_type_promotion` doc in `parquet_support.rs` says the flag mirrors `spark.comet.schemaEvolution.enabled`, but that config is gone (#4298) and the flag comes from `ShimCometConf`. 4. `CometCollectSet` decides SPARK-57298 with four separate `isSpark42Plus` checks, in `getCompatibleNotes`, `getIncompatibleReasons`, `getSupportLevel` and `convert`. A fork that backports SPARK-57298 has to find and change all four, where the rule asks for one place. ### Describe the potential solution Rewrite the comments in items 1 to 3 to name the JIRA and the behavior. For `Regr`, point at `RegrSparkVersions` for the exact releases rather than restating them in the proto and Rust, so the list cannot drift again. Replace the four checks in item 4 with one named predicate, for example `CometCollectSet.normalizesFloatingPointInBuffer`, documented with SPARK-57298. Optionally, rename the native tests that carry a Spark version in their name, `test_timestamp_trunc_fine_spark42_overflow`, `signed_zeros_are_distinct_keys_before_spark_42`, `signed_zeros_share_a_key_from_spark_42` and `slope_var_x_null_filtering_differs_by_spark_version`, after the behavior or the JIRA, the way `r2_degenerate_cases_swapped_by_spark_55969` is named. ### Additional context The rename in #6891 also has to cover `SparkCastOptions::new_with_version`, the `cast_string_to_timestamp/spark4_legacy` benchmark group, and the `test_number_limit_depends_on_spark_version` test. -- 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]
