andygrove commented on code in PR #6456:
URL: https://github.com/apache/datafusion-comet/pull/6456#discussion_r4149555972
##########
native/spark-expr/src/iceberg_funcs/temporal.rs:
##########
@@ -307,6 +359,15 @@ mod tests {
// `(int)` narrowing wraps it exactly as `as i32` does.
(i64::MAX, 292_277, 3_507_324, 106_751_991, -1_732_919_508),
(i64::MIN, -292_278, -3_507_325, -106_751_992, 1_732_919_507),
+ // The lowest `i64` that ends in 999999, where moving the value a
second earlier would
+ // overflow.
+ (
+ i64::MIN + 775_807,
Review Comment:
You're right, that row floored to the same four values as the `i64::MIN`
row. In 60037f5713 I replaced it with -290307-01-01T00:00:00.999999, 999999
micros past the lowest year boundary in range, so all four transforms take the
adjustment there, where your value moves only the day and the hour. I checked
the expected values against Iceberg's `DateTimeUtil` and `Transforms` on 1.5.2,
1.8.1, 1.10.0 and 1.11.0, and your numbers for `-106_751_991 * MICROS_PER_DAY +
999_999` check out as well. With the fix turned off, the new row fails.
##########
native/core/src/execution/operators/iceberg_write.rs:
##########
@@ -3569,7 +3569,10 @@ mod iceberg_rust_transform_parity {
}
}
- /// `days` and `hours` are plain floor division on both sides, so the
whole domain agrees.
+ /// `days` and `hours` agree with iceberg-rust except on the pre-epoch
timestamps where
Review Comment:
Fixed both in d711633555, along with two more that a grep turned up: the
`PartitionSplitter` doc, and a test comment in `iceberg_partition_value.rs`
that still said delegating year and month would become possible again.
##########
spark/src/test/scala/org/apache/comet/CometIcebergSystemFunctionSuite.scala:
##########
@@ -174,6 +174,24 @@ class CometIcebergSystemFunctionSuite
}
}
+ test("years, months, days, and hours match Iceberg on pre-1970 timestamps
ending in .999999") {
Review Comment:
Moved to `sql-tests/iceberg/temporal_functions_pre_epoch.sql` in ccbe740e2c,
with a NULL row. Each query uses `expect_native(staticinvoke)`, which the Scala
version didn't check: a call routed through the codegen dispatcher runs
Iceberg's own Java and would match Spark whatever the native kernel returns.
The file fails without the fix. The native write test and the extensions suite
stay in Scala.
##########
spark/src/test/scala/org/apache/comet/CometIcebergSystemFunctionExtensionsSuite.scala:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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
+
+import org.apache.spark.SparkConf
+import org.apache.spark.sql.CometTestBase
+import org.apache.spark.sql.catalyst.expressions.ApplyFunctionExpression
+import org.apache.spark.sql.catalyst.plans.logical.Filter
+import org.apache.spark.sql.internal.SQLConf
+
+/**
+ * Iceberg's system functions once Iceberg's SQL extensions have rewritten
them.
+ *
+ * The extensions' `ReplaceStaticInvoke` rule turns a system-function call
that a filter compares
+ * with a constant from a `StaticInvoke` into an `ApplyFunctionExpression`, so
that Iceberg can
+ * push the comparison into its scan. Over any other source the filter stays,
and Comet evaluates
+ * it with the same native kernels as the `StaticInvoke` that
CometIcebergSystemFunctionSuite
+ * covers. `spark.sql.extensions` is static, so this path needs a suite of its
own.
+ */
+class CometIcebergSystemFunctionExtensionsSuite extends CometTestBase with
CometIcebergTestBase {
+
+ override protected def sparkConf: SparkConf =
+ super.sparkConf.set(
+ "spark.sql.extensions",
+ "org.apache.iceberg.spark.extensions.IcebergSparkSessionExtensions")
+
+ test("rewritten temporal filters match Iceberg on pre-1970 timestamps ending
in .999999") {
+ assume(icebergAvailable, "Iceberg not available in classpath")
+ withTempIcebergDir { warehouseDir =>
Review Comment:
Done in c5d00f76c8. The catalog helper, `preEpochTimestamps` and the
`pre_epoch` table now live in `CometIcebergTestBase`, and the four predicates
work on the shared rows, as you expected. The helper is called
`withHadoopCatalog`, because `CometIcebergWriteActionSuite` has its own private
`withIcebergCatalog`. The trait now declares a `CometTestBase` self-type, which
the eight suites that mix it in already satisfy.
--
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]
