Implement INTERSECT & INTERSECT DISTINCT #1135

xudong963 · 2021-10-16T13:21:32Z

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

xudong963 · 2021-10-16T13:29:14Z

As @Dandandan has mentioned in #1082 (comment), "Complication with using things like is distinct from will not supported by a equi-join implementation (which only supports A=B type of expressions)", so I am considering how to solve it.

xudong963 · 2021-10-31T12:26:24Z

Inspired by spark which has a condition expr in Join to show if null equal is safe. So I add a bool type named null_equal_safe to indicate if null equal is safe.

PTAL @alamb @Dandandan @houqp, thanks!

houqp · 2021-11-03T03:57:55Z

Am I correct that in the current design, the null_equal_safe argument will apply to all join key pairs? i.e. will we be able to have t1 JOIN t2 ON t1.col1 = t2.col1, t1.col2 <=> t2.col2?

xudong963 · 2021-11-03T05:06:27Z

Am I correct that in the current design, the null_equal_safe argument will apply to all join key pairs? i.e. will we be able to have t1 JOIN t2 ON t1.col1 = t2.col1, t1.col2 <=> t2.col2?

Yes, the null_equal_safe argument will apply to all join key pairs.

alamb · 2021-11-03T20:52:26Z

I am sorry -- I will review this PR tomorrow

…

On Wed, Nov 3, 2021 at 1:06 AM Carlos ***@***.***> wrote: Am I correct that in the current design, the null_equal_safe argument will apply to all join key pairs? i.e. will we be able to have t1 JOIN t2 ON t1.col1 = t2.col1, t1.col2 <=> t2.col2? Yes, the null_equal_safe argument will apply to all join key pairs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1135 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADXZMKGWTP33NKTQAS6VTTUKC7N3ANCNFSM5GDUT2SA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

alamb

This is great work @xudong963 . I am really sorry for the delay in review

I have a few suggestions related to the external API (aka adding a null_equals_safe parameter) which I think should be taken into account, but otherwise this is looking very nice 👌

alamb · 2021-11-04T12:48:20Z

datafusion/src/dataframe.rs

@@ -211,6 +211,7 @@ pub trait DataFrame: Send + Sync {
        join_type: JoinType,
        left_cols: &[&str],
        right_cols: &[&str],
+        null_equal_safe: bool,


I think we should be documenting what null_equal_safe means in this public API

alamb · 2021-11-04T12:54:12Z

datafusion/src/logical_plan/builder.rs

@@ -488,6 +488,7 @@ impl LogicalPlanBuilder {
        right: &LogicalPlan,


Likewise I think we should add a doc comment about what null_equal_safe means in this context.

Given how specialized the use case of null_equal_safe is, what do you think about adding a specialized API for it. This would hide some of this complexity from most users of DataFusion.

So something like

/// Apply a join with on constraint and specified null equality. /// If null_equal_safe is true then ... pub fn join_detailed( &self, right: &LogicalPlan, join_type: JoinType, join_keys: (Vec<impl Into<Column>>, Vec<impl Into<Column>>, null_equal_safe: bool) { ... } /// Apply a join with on constraint pub fn join( &self, right: &LogicalPlan, join_type: JoinType, join_keys: (Vec<impl Into<Column>>, Vec<impl Into<Column>>) { self.join_detailed(right, join_type, join_keys, false) }

Nice idea! Then we can call join_detailed in INTESECT and other places keep original.

alamb · 2021-11-04T12:55:28Z

datafusion/src/logical_plan/plan.rs

@@ -135,6 +135,8 @@ pub enum LogicalPlan {
        join_constraint: JoinConstraint,
        /// The output schema, containing fields from the left and right inputs
        schema: DFSchemaRef,
+        /// If null_equal_safe is true, null == null.
+        null_equal_safe: bool,


I suggest naming this field (and all other instances of null_equal_safe) to null_equal_null to make its meaning clearer

alamb · 2021-11-04T12:58:31Z

datafusion/tests/sql.rs

+    let expected: &[&[&str]] = &[];
+
+    let mut ctx = create_join_context_qualified().unwrap();
+    let actual = execute(&mut ctx, sql).await;


is there a reason you didn't use execute_to_batches! and assert_batches_eq! in the first two tests and then did not use them in the subsequent tests? In other words, why are the tests inconsistent?

The first two tests contain NULL in results. I noticed the return value format of execute is clearer.

https://github.com/apache/arrow-datafusion/blob/3d4db909e83ea8354e9b9134c20b1cbef06a5beb/datafusion/tests/sql.rs#L3453

alamb · 2021-11-04T12:58:54Z

datafusion/tests/sql.rs

+    let mut ctx = create_join_context_qualified().unwrap();
+    let actual = execute(&mut ctx, sql).await;
+
+    // left and right shouldn't match anything


This comment appears to be incorrect

alamb · 2021-11-04T12:59:29Z

datafusion/tests/sql.rs

+async fn test_intersect_distinct() -> Result<()> {
+    let mut ctx = ExecutionContext::new();
+    register_alltypes_parquet(&mut ctx).await;
+    // execute the query


alamb · 2021-11-04T12:59:47Z

datafusion/src/sql/planner.rs

        let sql = "SELECT order_id from orders EXCEPT SELECT order_id FROM orders";
        let err = logical_plan(sql).expect_err("query should have failed");
        assert_eq!(
-            "NotImplemented(\"Only UNION ALL and UNION [DISTINCT] are supported, found EXCEPT\")",
+            "NotImplemented(\"Only UNION ALL and UNION [DISTINCT] and INTERSECT and INTERSECT [DISTINCT] are supported, found EXCEPT\")",


alamb · 2021-11-04T13:03:09Z

datafusion/src/sql/planner.rs

+                    let distinct_left_plan = LogicalPlanBuilder::from(left_plan).distinct()?.build()?;
+                    let join_keys = distinct_left_plan.schema().fields().iter().zip(right_plan.schema().fields().iter()).map(|(left_field, right_field)| ((Column::from_name(left_field.name())), (Column::from_name(right_field.name())))).unzip();
+                    LogicalPlanBuilder::from(distinct_left_plan).join(&right_plan, JoinType::Semi, join_keys, true)?.build()


This is just a cleanliness suggestion, definitely not needed:

Suggested change

let distinct_left_plan = LogicalPlanBuilder::from(left_plan).distinct()?.build()?;

let join_keys = distinct_left_plan.schema().fields().iter().zip(right_plan.schema().fields().iter()).map(|(left_field, right_field)| ((Column::from_name(left_field.name())), (Column::from_name(right_field.name())))).unzip();

LogicalPlanBuilder::from(distinct_left_plan).join(&right_plan, JoinType::Semi, join_keys, true)?.build()

let join_keys = distinct_left_plan

.schema()

.fields()

.iter()

.zip(right_plan.schema().fields().iter())

.map(|(left_field, right_field)| {

((Column::from_name(left_field.name())),

(Column::from_name(right_field.name())))

})

.unzip();

LogicalPlanBuilder::from(left_plan)

.distinct()?

.build()?

.join(&right_plan, JoinType::Semi, join_keys, true)?

.build()

Good caught!

alamb · 2021-11-04T13:03:33Z

datafusion/src/physical_plan/hash_join.rs

@@ -499,6 +510,8 @@ struct HashJoinStream {
    join_metrics: HashJoinMetrics,
    /// Information of index and left / right placement of columns
    column_indices: Vec<ColumnIndex>,
+    ///


seems like a missing doc comment

alamb · 2021-11-04T13:07:40Z

datafusion/src/physical_plan/hash_join.rs

        let left_array = $l.as_any().downcast_ref::<$array_type>().unwrap();
        let right_array = $r.as_any().downcast_ref::<$array_type>().unwrap();

        match (left_array.is_null($left), right_array.is_null($right)) {
            (false, false) => left_array.value($left) == right_array.value($right),
+            (true, true) => {


@Dandandan what do you think about the potential performance impact of doing this check for each input element?

I think it is fine but maybe there are some benchmarks we could run to find out

I am concerned bout the performance impact.
It would expect it to be optimized away, as it's using a macro for this and I would expect the branch to be removed based on the boolean constant, but not sure about that.

Also the is_null check and downcasting per item, I would expect this has a potential higher impact, even if the added code has some non-zero cost.

xudong963 · 2021-11-04T15:03:18Z

Thanks for your review @alamb. I'll fix the PR tomorrow.

xudong963 · 2021-11-05T13:33:42Z

Fixed! PTAL @alamb

Dandandan · 2021-11-05T14:52:45Z

datafusion/src/physical_plan/hash_join.rs

        let left_array = $l.as_any().downcast_ref::<$array_type>().unwrap();
        let right_array = $r.as_any().downcast_ref::<$array_type>().unwrap();

        match (left_array.is_null($left), right_array.is_null($right)) {
            (false, false) => left_array.value($left) == right_array.value($right),
+            (true, true) => {
+                if $null_equal_null {


This can be !$null_equal_null instead (without if/else)

Good point, I think you mean $null_equal_null not !$null_equal_null 😁

Yes, I thought true/false were swapped in if/else, but probably I had that incorrect 👍

xudong963 · 2021-11-06T04:20:30Z

@houqp Would you like to take another look? 😄

houqp

LGTM!

Dandandan · 2021-11-06T10:03:00Z

datafusion/src/physical_plan/hash_join.rs

@@ -120,6 +120,8 @@ pub struct HashJoinExec {
    metrics: ExecutionPlanMetricsSet,
    /// Information of index and left / right placement of columns
    column_indices: Vec<ColumnIndex>,
+    /// If null_equal_null is true, null == null else null != null


Nit, what about null_equals_null?

I agree, we should keep preciseness! 👍🏻

Dandandan

One minor comment, but this seems good to go to me! Awesome @xudong963 !

alamb

This is really nice -- thank you @xudong963

alamb · 2021-11-06T10:52:48Z

Will merge when tests pass

github-actions bot added datafusion Changes in the datafusion crate sql SQL Planner labels Oct 16, 2021

xudong963 force-pushed the impl_intersect branch from f28d166 to d93d516 Compare October 31, 2021 12:13

github-actions bot added the ballista label Oct 31, 2021

xudong963 force-pushed the impl_intersect branch from d93d516 to 176e61d Compare October 31, 2021 12:17

xudong963 changed the title ~~[WIP] Implement INTERSECT & INTERSECT DISTINCT~~ Implement INTERSECT & INTERSECT DISTINCT Oct 31, 2021

xudong963 force-pushed the impl_intersect branch from 176e61d to 79438a5 Compare October 31, 2021 12:31

houqp requested review from Dandandan and alamb November 1, 2021 07:30

rdettai mentioned this pull request Nov 2, 2021

execution::context::sql should support creating partitioned dataset #1220

Closed

alamb reviewed Nov 4, 2021

View reviewed changes

xudong963 force-pushed the impl_intersect branch from 79438a5 to 59102c0 Compare November 4, 2021 15:02

xudong963 force-pushed the impl_intersect branch from 59102c0 to 0140de5 Compare November 5, 2021 13:21

xudong963 mentioned this pull request Nov 5, 2021

Implement the rest of Set Operators: INTERSECT, EXCEPT, etc #1082

Closed

4 tasks

Dandandan reviewed Nov 5, 2021

View reviewed changes

xudong963 force-pushed the impl_intersect branch 3 times, most recently from f896cba to 06418f2 Compare November 6, 2021 03:31

houqp approved these changes Nov 6, 2021

View reviewed changes

houqp mentioned this pull request Nov 6, 2021

Support non-equi join (e.g. ON clause) in Dataframe API #1254

Closed

houqp added the enhancement New feature or request label Nov 6, 2021

Dandandan reviewed Nov 6, 2021

View reviewed changes

Dandandan approved these changes Nov 6, 2021

View reviewed changes

fix comments

3679f74

xudong963 force-pushed the impl_intersect branch from 06418f2 to 3679f74 Compare November 6, 2021 10:31

alamb approved these changes Nov 6, 2021

View reviewed changes

alamb merged commit 2b24b89 into apache:master Nov 6, 2021

xudong963 deleted the impl_intersect branch November 6, 2021 13:18

xudong963 mentioned this pull request Nov 12, 2021

Add integration comparison tests with psql for intersect & except #1286

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement INTERSECT & INTERSECT DISTINCT #1135

Implement INTERSECT & INTERSECT DISTINCT #1135

xudong963 commented Oct 16, 2021 •

edited

Loading

xudong963 commented Oct 16, 2021

xudong963 commented Oct 31, 2021

houqp commented Nov 3, 2021 •

edited

Loading

xudong963 commented Nov 3, 2021

alamb commented Nov 3, 2021 via email

alamb left a comment

alamb Nov 4, 2021

alamb Nov 4, 2021

xudong963 Nov 5, 2021

alamb Nov 4, 2021

alamb Nov 4, 2021

xudong963 Nov 5, 2021

alamb Nov 4, 2021

alamb Nov 4, 2021

alamb Nov 4, 2021

alamb Nov 4, 2021

xudong963 Nov 5, 2021

alamb Nov 4, 2021

alamb Nov 4, 2021

Dandandan Nov 5, 2021

Dandandan Nov 5, 2021

xudong963 commented Nov 4, 2021

xudong963 commented Nov 5, 2021

Dandandan Nov 5, 2021

xudong963 Nov 5, 2021

Dandandan Nov 5, 2021

xudong963 commented Nov 6, 2021

houqp left a comment

Dandandan Nov 6, 2021

xudong963 Nov 6, 2021

Dandandan left a comment

alamb left a comment

alamb commented Nov 6, 2021

		@@ -488,6 +488,7 @@ impl LogicalPlanBuilder {
		right: &LogicalPlan,

Implement INTERSECT & INTERSECT DISTINCT #1135

Implement INTERSECT & INTERSECT DISTINCT #1135

Conversation

xudong963 commented Oct 16, 2021 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

xudong963 commented Oct 16, 2021

xudong963 commented Oct 31, 2021

houqp commented Nov 3, 2021 • edited Loading

xudong963 commented Nov 3, 2021

alamb commented Nov 3, 2021 via email

alamb 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

xudong963 commented Nov 4, 2021

xudong963 commented Nov 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xudong963 commented Nov 6, 2021

houqp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dandandan left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb commented Nov 6, 2021

xudong963 commented Oct 16, 2021 •

edited

Loading

houqp commented Nov 3, 2021 •

edited

Loading