comphead commented on code in PR #6456:
URL: https://github.com/apache/datafusion-comet/pull/6456#discussion_r4146868050
##########
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:
Nit: a couple of doc comments the PR doesn't touch still describe the old
split. The one on `iceberg_rust_years_follow_the_timezone_tag` below says
`days` and `hours` could be delegated to iceberg-rust.
`SparkIcebergTemporalTransform::transform` in `temporal.rs` says the writer
uses it for `year` and `month` only.
##########
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:
Nit: does this row reach the new branch? Its remainder is 71_945_999_999
within the day and 3_545_999_999 within the hour, so I think a plain floor
gives the same four values as the `i64::MIN` row above. A value that does hit
it near the low end is `-106_751_991 * MICROS_PER_DAY + 999_999`. I haven't run
it, but I'd expect day -106_751_992 and hour 1_732_919_511 there, where a floor
gives -106_751_991 and 1_732_919_512.
##########
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:
Nit: the projection and filter queries here read a plain parquet table, so
would a SQL file test work? `sql-tests/iceberg/metadata_column_partition.sql`
registers an Iceberg catalog with `-- Config:` lines. It would also make it
easy to add a NULL row. The native write test and the extensions suite look
like they need to stay in Scala.
##########
native/core/src/execution/operators/iceberg_partition_value.rs:
##########
@@ -35,20 +35,31 @@ use iceberg::{Error, ErrorKind, Result};
/// Computes a batch's partition values: one row of the partition struct per
input row.
///
-/// Matches iceberg-rust's `PartitionValueCalculator` except for `year` and
`month` over a `date`,
-/// `timestamp`, or `timestamptz` source. iceberg-rust splits the calendar
with Arrow's `date_part`,
-/// which returns NULL for anything `chrono` cannot represent -- past year
262142 -- whereas
-/// iceberg-java's `DateTimeUtil` goes through `LocalDate` and covers every
Spark date (to year
-/// 5881580) and timestamp (to year 294247). The NULL did not fail the write:
the data file was
-/// committed claiming a NULL partition for rows whose source value is not NULL
-/// (apache/datafusion-comet#6145). Those two transforms go through Comet's
`iceberg_years` /
-/// `iceberg_months` kernels instead, the ones the sort in front of a
clustered write runs, which
-/// are pinned against iceberg-java over the whole domain. Wherever `chrono`
can represent the date
-/// the two implementations agree, so every value iceberg-rust could compute
is unchanged.
+/// Matches iceberg-rust's `PartitionValueCalculator` except for the time
transforms of a `date`,
+/// `timestamp`, or `timestamptz` source, which go through Comet's
`iceberg_years` /
+/// `iceberg_months` / `iceberg_days` / `iceberg_hours` kernels instead: the
ones the sort in front
+/// of a clustered write runs, pinned against iceberg-java's `DateTimeUtil`
over the whole domain.
+/// iceberg-rust's transforms differ from iceberg-java's in three ways:
///
-/// `day` and `hour` stay on iceberg-rust: they are floor divisions of the
epoch value and never
-/// consult the calendar. So do the nanosecond timestamp types, whose `i64`
range (years 1677 to
-/// 2262) lies inside `chrono`'s and which Comet's kernels do not accept.
+/// - `year` and `month` split the calendar with Arrow's `date_part`, which
returns NULL for
+/// anything `chrono` cannot represent -- past year 262142 -- whereas
iceberg-java goes through
+/// `LocalDate` and covers every Spark date (to year 5881580) and timestamp
(to year 294247). The
+/// NULL did not fail the write: the data file was committed claiming a NULL
partition for rows
+/// whose source value is not NULL (apache/datafusion-comet#6145).
+/// - All four floor a pre-epoch timestamp that lies exactly 999999
microseconds into a unit, which
+/// iceberg-java puts in the unit before, so `1969-01-01T00:00:00.999999`
belongs in the 1968
+/// partitions (apache/datafusion-comet#6426).
+/// - `day` moves a timestamp from the last second of a day before 1969-12-31
into the next day,
Review Comment:
Question: is there an iceberg-rust issue for the truncating `day`? I
couldn't find one. If not, would it be worth filing one and linking it here, as
is done for apache/iceberg-rust#3142 on the year and month tag? That would tell
the next reader when this special case can go.
##########
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:
Nit: this repeats the catalog setup and the `pre_epoch` table that
`withIcebergCatalog` and `withPreEpochTable` provide in
`CometIcebergSystemFunctionSuite`. Would it make sense to move those helpers
and `preEpochTimestamps` to `CometIcebergTestBase` so both suites share one
fixture? I haven't run it, but I expect the four predicates here to work on the
shared rows too.
--
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]
