Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upgrade to plk 0.14.6 + add UNION support #56

Merged
merged 4 commits into from
Jul 22, 2024
Merged

Upgrade to plk 0.14.6 + add UNION support #56

merged 4 commits into from
Jul 22, 2024

Conversation

alancai98
Copy link
Member

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@alancai98 alancai98 requested a review from RCHowell July 12, 2024 22:25
@alancai98 alancai98 self-assigned this Jul 12, 2024
@alancai98
Copy link
Member Author

Verified the transpiled test queries for each of the dialects.

For reference, commands for the table creation:

Redshift

CREATE TABLE SIMPLE_T (
    a BOOL,
    b INT4,
    c VARCHAR
);

INSERT INTO SIMPLE_T VALUES(
    true,
    1,
    'foo'
);

Spark

CREATE TABLE SIMPLE_T AS (SELECT true AS a, 2 AS b, 'foo' AS c);

Trino

WITH SIMPLE_T AS (
	SELECT true AS a,
		2 AS b,
		'foo' AS c
)

alancai98
alancai98 commented Jul 12, 2024
View reviewed changes
src/main/kotlin/org/partiql/scribe/shell/ShellHighlighter.kt
import org.jline.reader.Highlighter
import org.jline.reader.LineReader
import org.jline.utils.AttributedString
import org.jline.utils.AttributedStringBuilder
import org.jline.utils.AttributedStyle
import org.partiql.parser.antlr.PartiQLParser
import org.partiql.parser.antlr.PartiQLTokens
import org.partiql.parser.thirdparty.antlr.v4.runtime.BaseErrorListener
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in this file are due to upgrading to PLK 0.14.6, which internalized the ANTLR dependency. ANTLR runtime dependencies are available under org.partiql.parser.thirdparty.antlr.v4.runtime rather than org.antlr.v4.runtime.

src/test/resources/outputs/spark/operators/between.sql
@@ -1,23 +1,23 @@
--#[between-00]
-- between(decimal, int32, int32)
SELECT `T_DECIMALS`.`da` AS `da` FROM `default`.`T_DECIMALS` AS `T_DECIMALS` WHERE `T_DECIMALS`.`da` BETWEEN -1 AND 1;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each dialect had some failing BETWEEN tests following the upgrade to 0.14.6. These were unrelated to the UNION changes.

src/main/kotlin/org/partiql/scribe/sql/SqlDialect.kt
@@ -89,6 +89,13 @@ abstract class SqlDialect : AstBaseVisitor<SqlBlock, SqlBlock>() {
t = t concat ")"
t
}
node is Expr.BagOp -> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy over a change from the PLK PR partiql/partiql-lang-kotlin#1506 since Scribe uses its own-defined SqlDialect.

src/main/kotlin/org/partiql/scribe/sql/RexConverter.kt Outdated
@@ -279,7 +328,7 @@ public open class RexConverter(
val newRexConverter = RexConverter(transform, Locals(relProject.input.type.schema))
val type = constructor.type as? StructType ?: return null
if (type.constraints.contains(TupleConstraint.Open(false))
.not() || type.constraints.contains(TupleConstraint.Ordered).not()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too sure why the previous un-planning (plan -> AST) for SELECT VALUE to SQL select required the tuple to be ordered. Was giving some issues with the plan with the Rel set ops since they were unordered.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SELECT VALUE requires orderedness because SQL tuples are ordered. That is, SELECT VALUE is only equivalent to SQL SELECT if the constructor is an ordered tuple.

SQL UNION/INTERSECT/EXCEPT should preserve the ordereness, as schema is known.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks for the context. I added back the ordered tuple check in b543453 (#56).

src/main/kotlin/org/partiql/scribe/targets/redshift/RedshiftFeatures.kt Show resolved Hide resolved
@alancai98 alancai98 mentioned this pull request Jul 12, 2024
Merged
RCHowell
RCHowell requested changes Jul 15, 2024
View reviewed changes
src/main/kotlin/org/partiql/scribe/sql/RexConverter.kt Outdated
@@ -279,7 +328,7 @@ public open class RexConverter(
val newRexConverter = RexConverter(transform, Locals(relProject.input.type.schema))
val type = constructor.type as? StructType ?: return null
if (type.constraints.contains(TupleConstraint.Open(false))
.not() || type.constraints.contains(TupleConstraint.Ordered).not()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SELECT VALUE requires orderedness because SQL tuples are ordered. That is, SELECT VALUE is only equivalent to SQL SELECT if the constructor is an ordered tuple.

SQL UNION/INTERSECT/EXCEPT should preserve the ordereness, as schema is known.

src/main/kotlin/org/partiql/scribe/targets/redshift/RedshiftFeatures.kt Show resolved Hide resolved
@alancai98 alancai98 requested a review from RCHowell July 15, 2024 22:54
@RCHowell RCHowell merged commit 4edc270 into main Jul 22, 2024
@RCHowell RCHowell deleted the add-union branch July 22, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RCHowell RCHowell RCHowell approved these changes

Assignees

@alancai98 alancai98

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support UNION in transpilation
2 participants
@alancai98 @RCHowell