fix: Timestamp with timezone not considered `join on` #8150

acking-you · 2023-11-13T09:35:54Z

Which issue does this PR close?

part of #8147

Rationale for this change

The join filters all use the ExtractEquijoinPredicate optimizer to make the eq expression the key for the join on, and then the presence of join on determines whether to use HashJoinExec or NestedJoinExec. In this process, can _hash function determines whether an expression type can be hashed, but currently this function does not support timestamps with timezone.

What changes are included in this PR?

Modified the can_hash function to support timestamps with timezone.

Are these changes tested?

Are there any user-facing changes?

No

alamb · 2023-11-13T18:33:00Z

Thank you @acking-you - can you please add a test for this behavior -- perhaps in https://github.com/apache/arrow-datafusion/blob/main/datafusion/sqllogictest/test_files/joins.slt ?

acking-you · 2023-11-14T06:30:31Z

Thank you @acking-you - can you please add a test for this behavior -- perhaps in https://github.com/apache/arrow-datafusion/blob/main/datafusion/sqllogictest/test_files/joins.slt ?

👌 I'm done testing.

Dandandan · 2023-11-14T08:51:23Z

datafusion/sqllogictest/test_files/joins.slt

@@ -2468,6 +2489,14 @@ SELECT * FROM test_timestamps_table as t1 JOIN (SELECT * FROM test_timestamps_ta
 2018-11-13T17:11:10.011375885 2018-11-13T17:11:10.011375 2018-11-13T17:11:10.011 2018-11-13T17:11:10 Row 0 2018-11-13T17:11:10.011375885 2018-11-13T17:11:10.011375 2018-11-13T17:11:10.011 2018-11-13T17:11:10 Row 0
 2021-01-01T05:11:10.432 2021-01-01T05:11:10.432 2021-01-01T05:11:10.432 2021-01-01T05:11:10 Row 3 2021-01-01T05:11:10.432 2021-01-01T05:11:10.432 2021-01-01T05:11:10.432 2021-01-01T05:11:10 Row 3

+# test timestamp with timezone join on nanos datatype
+query PPPPTPPPPT rowsort
+SELECT * FROM test_timestamps_tz_table as t1 JOIN (SELECT * FROM test_timestamps_tz_table ) as t2 ON t1.nanos = t2.nanos;


Suggested change

SELECT * FROM test_timestamps_tz_table as t1 JOIN (SELECT * FROM test_timestamps_tz_table ) as t2 ON t1.nanos = t2.nanos;

SELECT * FROM test_timestamps_tz_table AS t1 JOIN test_timestamps_tz_table AS t2 ON t1.nanos = t2.nanos;

The original test on timestamp used as, do we need to change it to AS?

Dandandan · 2023-11-14T08:51:42Z

datafusion/sqllogictest/test_files/joins.slt

@@ -2476,6 +2505,14 @@ SELECT * FROM test_timestamps_table as t1 JOIN (SELECT * FROM test_timestamps_ta
 2018-11-13T17:11:10.011375885 2018-11-13T17:11:10.011375 2018-11-13T17:11:10.011 2018-11-13T17:11:10 Row 0 2018-11-13T17:11:10.011375885 2018-11-13T17:11:10.011375 2018-11-13T17:11:10.011 2018-11-13T17:11:10 Row 0
 2021-01-01T05:11:10.432 2021-01-01T05:11:10.432 2021-01-01T05:11:10.432 2021-01-01T05:11:10 Row 3 2021-01-01T05:11:10.432 2021-01-01T05:11:10.432 2021-01-01T05:11:10.432 2021-01-01T05:11:10 Row 3

+# test timestamp with timezone join on micros datatype
+query PPPPTPPPPT rowsort
+SELECT * FROM test_timestamps_tz_table as t1 JOIN (SELECT * FROM test_timestamps_tz_table ) as t2 ON t1.micros = t2.micros


Suggested change

SELECT * FROM test_timestamps_tz_table as t1 JOIN (SELECT * FROM test_timestamps_tz_table ) as t2 ON t1.micros = t2.micros

SELECT * FROM test_timestamps_tz_table AS t1 JOIN test_timestamps_tz_table AS t2 ON t1.micros = t2.micros

Dandandan · 2023-11-14T08:52:01Z

datafusion/sqllogictest/test_files/joins.slt

@@ -2484,6 +2521,46 @@ SELECT * FROM test_timestamps_table as t1 JOIN (SELECT * FROM test_timestamps_ta
 2018-11-13T17:11:10.011375885 2018-11-13T17:11:10.011375 2018-11-13T17:11:10.011 2018-11-13T17:11:10 Row 0 2018-11-13T17:11:10.011375885 2018-11-13T17:11:10.011375 2018-11-13T17:11:10.011 2018-11-13T17:11:10 Row 0
 2021-01-01T05:11:10.432 2021-01-01T05:11:10.432 2021-01-01T05:11:10.432 2021-01-01T05:11:10 Row 3 2021-01-01T05:11:10.432 2021-01-01T05:11:10.432 2021-01-01T05:11:10.432 2021-01-01T05:11:10 Row 3

+# test timestamp with timezone join on millis datatype
+query PPPPTPPPPT rowsort
+SELECT * FROM test_timestamps_tz_table as t1 JOIN (SELECT * FROM test_timestamps_tz_table ) as t2 ON t1.millis = t2.millis


Suggested change

SELECT * FROM test_timestamps_tz_table as t1 JOIN (SELECT * FROM test_timestamps_tz_table ) as t2 ON t1.millis = t2.millis

SELECT * FROM test_timestamps_tz_table AS t1 JOIN test_timestamps_tz_table AS t2 ON t1.millis = t2.millis

comphead

This PR can be affected by #8193

alamb

Nice work @acking-you -- thank you very much 🙏

alamb · 2023-11-17T19:30:53Z

datafusion/expr/src/utils.rs

@@ -901,7 +901,7 @@ pub fn can_hash(data_type: &DataType) -> bool {
        DataType::UInt64 => true,
        DataType::Float32 => true,
        DataType::Float64 => true,
-        DataType::Timestamp(time_unit, None) => match time_unit {
+        DataType::Timestamp(time_unit, _) => match time_unit {


it seems like there is no reason to check the timeunit either (as all branches are true)

However that is not something introduced in this PR

alamb · 2023-11-17T19:34:14Z

datafusion/sqllogictest/test_files/joins.slt

+----TableScan: test_timestamps_tz_table projection=[nanos, micros, millis, secs, names]
+physical_plan
+CoalesceBatchesExec: target_batch_size=2
+--HashJoinExec: mode=Partitioned, join_type=Inner, on=[(millis@2, millis@2)]


I verified that this test covers the code, and without the changes in this PR and it results in NestedLoopsJoin 👍

+ NestedLoopJoinExec: join_type=Inner, filter=millis@0 = millis@1 + --RepartitionExec: partitioning=RoundRobinBatch(2), input_partitions=1 + ----MemoryExec: partitions=1, partition_sizes=[1] + --MemoryExec: partitions=1, partition_sizes=[1]

github-actions bot added the logical-expr Logical plan and expressions label Nov 13, 2023

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Nov 14, 2023

ackingliu added 2 commits November 14, 2023 14:36

fix: Timestamp with timezone not considerd in

d56e132

Add Test For Explain HashJoin On Timestamp with Tz

37564aa

acking-you force-pushed the fix/timestamp_join_on branch from 4ba7a8b to 37564aa Compare November 14, 2023 06:37

Dandandan reviewed Nov 14, 2023

View reviewed changes

alamb changed the title ~~fix: Timestamp with timezone not considerd join on~~ fix: Timestamp with timezone not considered join on Nov 14, 2023

comphead reviewed Nov 16, 2023

View reviewed changes

alamb approved these changes Nov 17, 2023

View reviewed changes

alamb merged commit bc0ed23 into apache:main Nov 17, 2023
23 checks passed

acking-you mentioned this pull request Nov 18, 2023

Timestamp types with timezone are not considered in Join's equivalent expression #8147

Closed

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Timestamp with timezone not considered `join on` #8150

fix: Timestamp with timezone not considered `join on` #8150

acking-you commented Nov 13, 2023

alamb commented Nov 13, 2023

acking-you commented Nov 14, 2023

Dandandan Nov 14, 2023

acking-you Nov 15, 2023

Dandandan Nov 14, 2023

Dandandan Nov 14, 2023

comphead left a comment

alamb left a comment

alamb Nov 17, 2023

alamb Nov 17, 2023

	SELECT * FROM test_timestamps_tz_table as t1 JOIN (SELECT * FROM test_timestamps_tz_table ) as t2 ON t1.nanos = t2.nanos;
	SELECT * FROM test_timestamps_tz_table AS t1 JOIN test_timestamps_tz_table AS t2 ON t1.nanos = t2.nanos;

fix: Timestamp with timezone not considered join on #8150

fix: Timestamp with timezone not considered join on #8150

Conversation

acking-you commented Nov 13, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

alamb commented Nov 13, 2023

acking-you commented Nov 14, 2023

Dandandan Nov 14, 2023

Choose a reason for hiding this comment

acking-you Nov 15, 2023

Choose a reason for hiding this comment

Dandandan Nov 14, 2023

Choose a reason for hiding this comment

Dandandan Nov 14, 2023

Choose a reason for hiding this comment

comphead left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Nov 17, 2023

Choose a reason for hiding this comment

alamb Nov 17, 2023

Choose a reason for hiding this comment

fix: Timestamp with timezone not considered `join on` #8150

fix: Timestamp with timezone not considered `join on` #8150