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.

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

PartiQL Parser defaults ordering_spec and nulls_spec in the sort_spec AST #832

Closed
jpschorr opened this issue Oct 7, 2022 · 0 comments · Fixed by #834
Closed

PartiQL Parser defaults ordering_spec and nulls_spec in the sort_spec AST #832

jpschorr opened this issue Oct 7, 2022 · 0 comments · Fixed by #834
Labels
bug Something isn't working

Comments

@jpschorr
Copy link
Contributor

jpschorr commented Oct 7, 2022

Description

Since #554, the Parser has defaulted the produced AST's ordering_spec and nulls_spec in the sort_spec.

The AST allows both ordering_spec and nulls_spec to be null within the sort_spec:

        // <expr> [ASC | DESC] [NULLS FIRST | NULLS LAST]
        (product sort_spec expr::expr ordering_spec::(? ordering_spec) nulls_spec::(? nulls_spec))

        // Desired ordering spec: ASC or DESC
        (sum ordering_spec
            (asc)
            (desc)
        )

        // Desired null/missing ordering spec: NULLS FIRST or NULLS LAST
        (sum nulls_spec
             (nulls_first)
             (nulls_last)
        )

-- from https://github.com/partiql/partiql-lang-kotlin/blob/main/lang/resources/org/partiql/type-domains/partiql.ion#L341-L353

But the parser automatically populates these parts of the AST with the appropriate defaults if not provided, as asserted by the following test example:

fun orderByWithAscAndDescShouldProduceDefaultNullsSpec() = assertExpression("SELECT x FROM tb ORDER BY rk1 desc, rk2 asc, rk3 asc, rk4 desc") {
select(
project = projectX,
from = scan(id("tb")),
order = orderBy(
listOf(
sortSpec(id("rk1"), desc(), nullsFirst()),
sortSpec(id("rk2"), asc(), nullsLast()),
sortSpec(id("rk3"), asc(), nullsLast()),
sortSpec(id("rk4"), desc(), nullsFirst())
)
)
)
}

So, though nullable, these fields are effectively never null.

This can cause issues for customers who wish to pre-process or validate the AST, as it is unclear whether the sort spec and nulls spec are user-supplied or defaulted.

Expected Behavior

It should be possible to move the defaulting the creation of the evaluator and preserve the query-writers intent in the AST.

Additional Context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant