Simplifies joins and fixes bugs #1438

johnedquinn · 2024-04-19T23:03:58Z

Relevant Issues

None

Description

Increases conformance by 4 tests (.07%). See report.
Simplifies joins by using Kotlin's helpful iterator inline function. This substantially simplifies our code at the cost of some performance. The reason I'm making this move is that there are bugs in the current implementation, and for the sake of our goal (conformance, not performance), I'm moving towards a simpler implementation. Once we pass all join tests, we can re-evaluate our implementation for performance reasons.
This PR now passes 8 additional tests, however, there are now 4 failing that were not previously failing. This is due to our inability to correctly coerce nulls to other types. Please disregard this issue as this is being handled in Initializes Datum and adds functionality to evaluator #1451.

Other Information

Updated Unreleased Section in CHANGELOG: NO
Any backward-incompatible changes? Maybe?
- There is one change that is worth noting. <struct evaluating to null>.a returns null. Is this right? IMO, it makes the most sense, and it makes the modelling of JOINs very very easy.
Any new external dependencies? NO
Do your changes comply with the Contributing Guidelines and Code Style Guidelines? YES

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov-commenter · 2024-04-19T23:13:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (v1@5b86afc). Learn more about missing BASE report.

Additional details and impacted files

@@          Coverage Diff          @@
##             v1    #1438   +/-   ##
=====================================
  Coverage      ?   13.70%           
  Complexity    ?       31           
=====================================
  Files         ?       29           
  Lines         ?     1729           
  Branches      ?      190           
=====================================
  Hits          ?      237           
  Misses        ?     1470           
  Partials      ?       22

Flag	Coverage Δ
CLI	`13.70% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

RCHowell · 2024-04-24T18:44:15Z

Why draft?

johnedquinn · 2024-04-24T18:46:41Z

Why draft?

Currently incorporating the change for check() that is causing some of the conformance tests for joins to fail.

partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelJoinOuterRight.kt

yliuuuu · 2024-05-11T01:39:04Z

partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelJoinOuterFull.kt

+     *     yield(null, rhsRecord)
+     * ```
+     *
+     * Note: The LHS and RHS must be sorted. TODO: We need to add sorting to the LHS and RHS


Could you elaborate on this please?

Updated KDoc.

partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelJoinOuterFull.kt

yliuuuu · 2024-05-11T01:48:34Z

partiql-plan/src/main/resources/partiql_plan.ion

+    // TODO: Specify that this is a LATERAL JOIN. Create a separate JOIN. Also, determine the allowable types of JOIN.
+    // For context: Oracle SQL doesn't allow ... FULL OUTER JOIN LATERAL ... or ... RIGHT OUTER JOIN LATERAL ...


PartiQL Spec section 5.7

SQL 2003 used the LATERAL keyword to correlate FROM clause items. In the interest of compatibility with SQL, PartiQL also allows the use of the keyword LATERAL, though it does not do anything more than the comma itself would do.
That is “l , LATERAL r” is equivalent to “l , r”.

This is: I don't think we need to support LATERAL JOIN. The spec is quite clear about there is no difference between left join and left lateral join, inner join and inner lateral join.

The spec is also quite clear about the fact that Full Join can not be lateral.

I think the only decision we have to make here is if we want right join to be lateral.

I vote no : )

Discussed offline a while back. I'd like to have different plan nodes for both types -- which corresponds to a different implementation in the evaluator. While yes, syntactically, one doesn't need to use the LATERAL keyword, it has semantic implementations. Upon analyzing the plan and seeing no references to bindings from the LHS, we can potentially use the normal join operator (and not the lateral join).

If we ever want to support that in the future, what would we call this plan node in the future? non_lateral_join? Since join is already taken and is used to semantically denote a lateral join?

Especially given that we are a library where DB implementers choose which features to support. Does their execution allow for joins? DynamoDB doesn't. What about lateral joins? Does every implementation support lateral joins in their execution? I'm not sure -- but I do know that lateral joins are expensive due to their correlated nature. I believe it limits the implementations that one can use. If I'm not mistaken, you are forced to use a variation of a nested loop join.

yliuuuu

Implementation looks good to me.

Had some small comments around test cases and kdoc.

partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelJoinOuterFull.kt

partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rel/RelJoinOuterRight.kt

yliuuuu · 2024-07-12T16:46:58Z

partiql-eval/src/test/kotlin/org/partiql/eval/internal/PartiQLEngineDefaultTest.kt

+            // RIGHT OUTER JOIN -- No Matches
+            SuccessTestCase(
+                input = """
+                    SELECT VALUE [lhs, rhs]
+                    FROM << 0, 1, 2 >> lhs
+                    RIGHT OUTER JOIN << 3, 4, 5 >> rhs
+                    ON lhs = rhs
+                """.trimIndent(),
+                expected = bagValue(
+                    listValue(int32Value(null), int32Value(3)),
+                    listValue(int32Value(null), int32Value(4)),
+                    listValue(int32Value(null), int32Value(5)),
+                )
+            ),


Perhaps add a couple tests for full outer join as well?

Also, would be nice if we can test the lateral properties of Left Join, and set up baseline for non-lateral behavior for Right and Full Join. Not sure if those should go to the evaluation test or the planning test (if not already exist)

I've been using the conformance tests for full outer join -- but I just updated and pushed additional tests for planning.

yliuuuu

Thanks for the additional test cases.

johnedquinn force-pushed the v1-conformance-join branch 2 times, most recently from 2b3236b to d114152 Compare April 23, 2024 22:47

johnedquinn mentioned this pull request Apr 27, 2024

Initializes Datum and adds functionality to evaluator #1451

Merged

johnedquinn force-pushed the v1-conformance-join branch from d114152 to 19886c3 Compare April 27, 2024 18:11

johnedquinn marked this pull request as ready for review April 27, 2024 18:12

johnedquinn mentioned this pull request May 2, 2024

[TRACKER] Up-to-date Conformance Tracker #1433

Closed

johnedquinn requested a review from yliuuuu May 10, 2024 18:59

yliuuuu reviewed May 11, 2024

View reviewed changes

johnedquinn force-pushed the v1-conformance-join branch from 19886c3 to 7d69467 Compare July 9, 2024 23:21

Simplifies joins and fixes bugs

7ac5b8f

johnedquinn force-pushed the v1-conformance-join branch from 7d69467 to 7ac5b8f Compare July 10, 2024 16:29

johnedquinn requested a review from yliuuuu July 10, 2024 16:30

yliuuuu reviewed Jul 12, 2024

View reviewed changes

johnedquinn added 2 commits July 12, 2024 12:41

Updates KDocs and adds lateral tests

45979a9

Utilizes the tests at head

1cd0272

yliuuuu approved these changes Jul 12, 2024

View reviewed changes

johnedquinn merged commit 35271b1 into partiql:v1 Jul 12, 2024
7 checks passed

johnedquinn deleted the v1-conformance-join branch July 12, 2024 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplifies joins and fixes bugs #1438

Simplifies joins and fixes bugs #1438

johnedquinn commented Apr 19, 2024 •

edited

Loading

codecov-commenter commented Apr 19, 2024 •

edited

Loading

RCHowell commented Apr 24, 2024

johnedquinn commented Apr 24, 2024

yliuuuu May 11, 2024

johnedquinn Jul 10, 2024

yliuuuu May 11, 2024

johnedquinn Jul 10, 2024

yliuuuu left a comment

yliuuuu Jul 12, 2024

johnedquinn Jul 12, 2024

yliuuuu left a comment

		// TODO: Specify that this is a LATERAL JOIN. Create a separate JOIN. Also, determine the allowable types of JOIN.
		// For context: Oracle SQL doesn't allow ... FULL OUTER JOIN LATERAL ... or ... RIGHT OUTER JOIN LATERAL ...

Simplifies joins and fixes bugs #1438

Simplifies joins and fixes bugs #1438

Conversation

johnedquinn commented Apr 19, 2024 • edited Loading

Relevant Issues

Description

Other Information

License Information

codecov-commenter commented Apr 19, 2024 • edited Loading

Codecov Report

RCHowell commented Apr 24, 2024

johnedquinn commented Apr 24, 2024

yliuuuu May 11, 2024

Choose a reason for hiding this comment

johnedquinn Jul 10, 2024

Choose a reason for hiding this comment

yliuuuu May 11, 2024

Choose a reason for hiding this comment

johnedquinn Jul 10, 2024

Choose a reason for hiding this comment

yliuuuu left a comment

Choose a reason for hiding this comment

yliuuuu Jul 12, 2024

Choose a reason for hiding this comment

johnedquinn Jul 12, 2024

Choose a reason for hiding this comment

yliuuuu left a comment

Choose a reason for hiding this comment

johnedquinn commented Apr 19, 2024 •

edited

Loading

codecov-commenter commented Apr 19, 2024 •

edited

Loading