feat(datafusion): Treat timestamp conversion functions like a cast. #945

omerhadari · 2025-02-05T15:23:29Z

Partially Solves #933

Note: This PR also fixes a bug that made Casts to Date which implicitly truncate the expression inside them create predicates with the raw expression, rather than with the truncated date. This could result in wrong predicates (see the relevant test in the PR).

omerhadari · 2025-02-05T15:52:51Z

EDIT: copying this message to the issue itself, since discussion started there.

@Fokko hope it's ok to tag :)

This PR contains the first (and easiest) part of the fix for the issue.

I wanted to ask, is there a way to express function within iceberg predicates? Is this even desired? The reason this could be beneficial is that sometimes you need access to the column value and then you could perform much better manifest elimination. A few examples I have in this context:

TO_DATE essentially converts the column to Timestamp, and then truncates to the nearest day. I cannot easily do that in the context of generating the predicate
TO_TIMESTAMP accepts format for strings, but I see no way to pass the format inside the predicate and use it correctly.

This also reveals what I think is a bug. In datafusion (as well as many engines) when you cast for example a string to a DATE, it truncates to the nearest day. Currently in the conversion function - the expression is simply extracted from the Cast.

See here:

If I understand correctly, this could cause wrong results for example for the query
SELECT * FROM table WHERE date_col > CAST('2025-01-01T00:10:00' AS DATE)

would result in the predicate date_col > '2025-01-01T00:10:00' which will filter out data files where 2025-01-01T00:00:00 < date_col < 2025-01-01T00:10:00 even though they are supposed to be included.

Would appreciate some guidance about how to tackle this issue of propagating more information, I don't think it makes sense in the scope of this PR but maybe I am missing something basic.

Fokko · 2025-02-06T14:06:27Z

Hey @omerhadari Thanks for working on this. Unfortunately, I'm afraid that this is a pretty complex task.

When we do like to_date(ts) in SQL, that maps to an Iceberg partition transform day(ts). What we do in Java, we convert the to_date(ts) into an UnboundTransform. This transform will be bound to the schema at some point. When you do: to_date(ts) >= 'date' 2021-01-01 the evaluator sees that it is both a date, and it can compare it directly against the partition values.

crates/integrations/datafusion/src/physical_plan/expr_to_predicate.rs

omerhadari · 2025-02-08T19:59:43Z

Updated the PR with the cases that were discussed to be within the scope for now (in this comment)

I think this makes sense for a single PR, though it doesn't solve the entire issue.

omerhadari · 2025-02-08T20:14:37Z

Hey @omerhadari Thanks for working on this. Unfortunately, I'm afraid that this is a pretty complex task.

When we do like to_date(ts) in SQL, that maps to an Iceberg partition transform day(ts). What we do in Java, we convert the to_date(ts) into an UnboundTransform. This transform will be bound to the schema at some point. When you do: to_date(ts) >= 'date' 2021-01-01 the evaluator sees that it is both a date, and it can compare it directly against the partition values.

Does it make sense to create an issue for supporting an equivalent to UnboundTransform here?

omerhadari · 2025-02-12T08:35:59Z

@Fokko will you be able to do another round of review?

liurenjie1024

Thanks @omerhadari for this pr, but I think the correct way to solve problems in this pr is to call expression simplification in datafusion rather hacking in this pr.

liurenjie1024 · 2025-02-12T10:00:43Z

crates/integrations/datafusion/src/physical_plan/expr_to_predicate.rs

@@ -119,7 +122,53 @@ fn to_iceberg_predicate(expr: &Expr) -> TransformedResult {
                _ => TransformedResult::NotTransformed,
            }
        }
-        Expr::Cast(c) => to_iceberg_predicate(&c.expr),
+        Expr::Cast(c) => {
+            if DataType::Date32 == c.data_type || DataType::Date64 == c.data_type {


I have concerns handling this case in such a way here. This is a process of simplifying expression in datafusion, which should call datafusion's expression simplification api.

This is less about simplification, rather than extracting the right information for Iceberg to optimize.

This is less about simplification, rather than extracting the right information for Iceberg to optimize.

Maybe the word simplification is a little confusing, I think constant folding is better here. The expressions in tests should be simplified by constant folding and replaced with a literal, which could be processed by iceberg.

Constant folding is being done I think, (e.g. TO_TIMESTAMP(2+2) => TO_TIMESTAMP(4)) but I am not sure it makes sense to turn TO_TIMESTAMP(<date_str>) to CAST(date_str AS TIMESTAMP) as a simplification in Datafusion. Currently the implementation of CAST and TO_TIMESTAMP is the same with no parameters, but I don't think turning TO_TIMESTAMP to CAST is a "simplification" necessarily. It may even be counter intuitive and effect errors in some weird ways, with no gains to readability or performance.

By simplification I don't mean converting TO_TIMESAMPT(<date_str>) to CAST(date_str AS TIMESTAMP), I mean it should be converting TO_TIMESAMPT(<date_str>) to <timestamp literal>, which should be done in constant folding.

I think datafusion's expression simplification api should have done this, you could see this example:
https://github.com/apache/datafusion/blob/132b21f02763cabbab85ab99e0e0c7fd6cd3d516/datafusion-examples/examples/expr_api.rs#L153
https://github.com/apache/datafusion/blob/132b21f02763cabbab85ab99e0e0c7fd6cd3d516/datafusion-examples/examples/expr_api.rs#L180

Ok, I don't mind it being in in DataFusion, I'll try opening an issue/PR there and see.

This doesn't seem a datafusion problem, maybe we should call the simplification api in the iceberg planner?

This would be redundant I think. We should call it in the tests to mimic production behaviour.

The current simplification API in DataFusion simplifies casts, not these specific function calls (TO_TIMESTAMP, TO_DATE) as far as I could tell.

I'm not sure about the details, but I guess in iceberg-datafusion planner we missed sth. It's fine for me to seek for help in datafusion community.

crates/integrations/datafusion/src/physical_plan/expr_to_predicate.rs

Fokko · 2025-02-12T14:53:57Z

crates/integrations/datafusion/src/physical_plan/expr_to_predicate.rs

+            if DataType::Date32 == c.data_type || DataType::Date64 == c.data_type {
+                match c.expr.as_ref() {
+                    Expr::Literal(ScalarValue::Utf8(Some(literal))) => {
+                        let date = literal.split('T').next();


I would be reluctant to do any string manipulation here.

Instead, I would add support for the datetime format in date_from_str:

iceberg-rust/crates/iceberg/src/spec/values.rs

Lines 1459 to 1466 in 75c7e8d

pub fn date_from_str<S: AsRef<str>>(s: S) -> Result<Self> {

let t = s.as_ref().parse::<NaiveDate>().map_err(|e| {

Error::new(

ErrorKind::DataInvalid,

format!("Can't parse date from string: {}", s.as_ref()),

)

.with_source(e)

})?;

@Fokko currently date_from_str supports datetime (i.e. 2025-01-01T11:11:11 will not be truncated).

I think this is a mistake given the name of the function (date_from_str and not datetime_from_str) and its documentation, but I don't think I can change its behaviour so drastically now right?

Do you suggest I change it, or create another one that is fix?

Fokko · 2025-02-12T14:54:57Z

crates/integrations/datafusion/src/physical_plan/expr_to_predicate.rs

+
+    #[test]
+    fn test_to_date_comparison_creates_predicate() {
+        let sql = "ts >= CAST('2023-01-05T11:11:11' AS DATE)";


I'm surprised to see that this works:

spark-sql (default)> SELECT CAST('2023-01-05T11:11:11' AS DATE); 2023-01-05

Personally I would have preferred an error, but nobody is asking me 😆

omerhadari · 2025-02-17T08:21:30Z

@liurenjie1024 @Fokko regardless of whether or not turning TO_TIMESTAMP/DATE into casts makes sense or should be done as a simplification in DataFusion, an important part in this PR is fixing the wrong handling of CAST(<datetime_str> AS DATE) expressions, which can cause incorrect query results. Would you like me to extract this part only as a separate PR so that it can be reviewed on its own?

Talking about what you can see here

liurenjie1024 · 2025-02-18T09:16:42Z

@liurenjie1024 @Fokko regardless of whether or not turning TO_TIMESTAMP/DATE into casts makes sense or should be done as a simplification in DataFusion, an important part in this PR is fixing the wrong handling of CAST(<datetime_str> AS DATE) expressions, which can cause incorrect query results. Would you like me to extract this part only as a separate PR so that it can be reviewed on its own?

Talking about what you can see here

Thanks @jonathanc-n I think it would make sense to fix the bug you mentioned, and it would be better to put it in another pr.

omerhadari · 2025-02-18T18:22:01Z

@liurenjie1024 @Fokko regardless of whether or not turning TO_TIMESTAMP/DATE into casts makes sense or should be done as a simplification in DataFusion, an important part in this PR is fixing the wrong handling of CAST(<datetime_str> AS DATE) expressions, which can cause incorrect query results. Would you like me to extract this part only as a separate PR so that it can be reviewed on its own?
Talking about what you can see here

Thanks @jonathanc-n I think it would make sense to fix the bug you mentioned, and it would be better to put it in another pr.

here

Fokko reviewed Feb 6, 2025

View reviewed changes

crates/integrations/datafusion/src/physical_plan/expr_to_predicate.rs Outdated Show resolved Hide resolved

omerhadari marked this pull request as ready for review February 8, 2025 19:59

omerhadari added 3 commits February 8, 2025 22:10

Support TO_TIMESTAMP like a cast

d3a2bdd

Remove explicit cast to timestamp from tests

0e07afb

Convert function calls to cast, deal with dates

735e7df

omerhadari force-pushed the feat/support-to-timestamp-and-to-date branch from 4907956 to 735e7df Compare February 8, 2025 20:10

omerhadari added 2 commits February 8, 2025 22:17

Fix comment

20044dc

Merge branch 'main' into feat/support-to-timestamp-and-to-date

dca0304

liurenjie1024 reviewed Feb 12, 2025

View reviewed changes

Fokko reviewed Feb 12, 2025

View reviewed changes

crates/integrations/datafusion/src/physical_plan/expr_to_predicate.rs Show resolved Hide resolved

Fokko reviewed Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(datafusion): Treat timestamp conversion functions like a cast. #945

feat(datafusion): Treat timestamp conversion functions like a cast. #945

omerhadari commented Feb 5, 2025 •

edited

Loading

omerhadari commented Feb 5, 2025 •

edited

Loading

Fokko commented Feb 6, 2025

omerhadari commented Feb 8, 2025

omerhadari commented Feb 8, 2025 •

edited

Loading

omerhadari commented Feb 12, 2025

liurenjie1024 left a comment

liurenjie1024 Feb 12, 2025

Fokko Feb 12, 2025

liurenjie1024 Feb 13, 2025

omerhadari Feb 13, 2025

liurenjie1024 Feb 17, 2025

liurenjie1024 Feb 17, 2025

omerhadari Feb 17, 2025

liurenjie1024 Feb 17, 2025

omerhadari Feb 17, 2025

liurenjie1024 Feb 17, 2025

Fokko Feb 12, 2025

Fokko Feb 12, 2025

omerhadari Feb 12, 2025

Fokko Feb 12, 2025

omerhadari Feb 13, 2025

omerhadari commented Feb 17, 2025 •

edited

Loading

liurenjie1024 commented Feb 18, 2025

omerhadari commented Feb 18, 2025

	pub fn date_from_str<S: AsRef<str>>(s: S) -> Result<Self> {
	let t = s.as_ref().parse::<NaiveDate>().map_err(\|e\| {
	Error::new(
	ErrorKind::DataInvalid,
	format!("Can't parse date from string: {}", s.as_ref()),
	)
	.with_source(e)
	})?;

feat(datafusion): Treat timestamp conversion functions like a cast. #945

Are you sure you want to change the base?

feat(datafusion): Treat timestamp conversion functions like a cast. #945

Conversation

omerhadari commented Feb 5, 2025 • edited Loading

omerhadari commented Feb 5, 2025 • edited Loading

Fokko commented Feb 6, 2025

omerhadari commented Feb 8, 2025

omerhadari commented Feb 8, 2025 • edited Loading

omerhadari commented Feb 12, 2025

liurenjie1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omerhadari commented Feb 17, 2025 • edited Loading

liurenjie1024 commented Feb 18, 2025

omerhadari commented Feb 18, 2025

omerhadari commented Feb 5, 2025 •

edited

Loading

omerhadari commented Feb 5, 2025 •

edited

Loading

omerhadari commented Feb 8, 2025 •

edited

Loading

omerhadari commented Feb 17, 2025 •

edited

Loading