Implement ORDER BY

[raganhan]

Just like the title says: add support for ORDER BY according to the specification.

[abbashus]

Need the ORDER BY support to understand the behaviour with NULL and MISSING values.

Right now we are getting this:

elasticdump --input "http://localhost:9200/table1" --output=$ | jq "._source" | bin/partiql --query="SELECT id FROM input_data ORDER BY id"
Unexpected token after expression

[raganhan]

ORDER BY is not currently supported by the reference implementation. The unexpected token is referring to the ORDER keyword.

[mustafaakin]

What is the reason this is labeled Large Task? Just wanted to help but it might be over my head.

[therapon]

That is a t-shirt size estimate for the task. The reason for the L tag is that ORDER BY needs to implement the ordering semantics per the spec (section 12 https://partiql.org/assets/PartiQL-Specification.pdf). While the spec lays out the ordering between PartiQL values, testing is also going to be on the heavy side to ensure the implementation's correctness.

Having said that, this is a task that will introduce you to a lot of the internals as well as a feature that we have seen interest from clients. Let us know if you would like to take a crack at it and we would do our best to assist in any way we can.

[mustafaakin]

Since #422 mentions ORDER BY is parsed but not evaluated, what would be steps to implement it? I'd be willing to contribute If I can.

[alancai98]

Hi @mustafaakin,

Sorry for the delay. The primary steps for implementing ORDER BY in the evaluator would involve:

1. Implementing and testing the ORDER BY semantics on values specified in the the spec (section 12: https://partiql.org/assets/PartiQL-Specification.pdf#page=44).
2. Implementing the compilation and evaluation in EvaluatingCompiler.kt

   partiql-lang-kotlin/lang/src/org/partiql/lang/eval/EvaluatingCompiler.kt

   Lines 1631 to 1636 in 7f07dc3

   	selectExpr.orderBy?.let {
   	err("ORDER BY is not supported in evaluator yet",
   	ErrorCode.EVALUATOR_FEATURE_NOT_SUPPORTED_YET,
   	errorContextFrom(selectExpr.metas).also { it[Property.FEATURE_NAME] = "ORDER BY" },
   	internal = false)
   	}

3. Adding additional tests to EvaluatorTestSuite or some test file in eval/

There are some additional caveats.

• We are in the process of deprecating ExprNode in the evaluator (PR Deprecate ExprNode in Evaluator #528) in favor of the PIG-generated PartiqlAst. That work should be merged to main sometime this week.
• We may also want to confirm the semantics defined in the spec definition for ORDER BY before implementation starts. We can collect some feedback by the end of the week if anything should be changed.

[alancai98]

To followup on the caveats,

We are in the process of deprecating ExprNode in the evaluator (PR #528) in favor of the PIG-generated PartiqlAst. That work should be merged to main sometime this week.

We are on track to finish the deprecation this week or early next week.

We may also want to confirm the semantics defined in the spec definition for ORDER BY before implementation starts. We can collect some feedback by the end of the week if anything should be changed.

Discussed with the team and the core semantics mentioned in section 12.2 (The PartiQL order-by less-than function) and 12.5 (Use of SELECT variables in ORDER BY for SQL compatibility) should be ready for implementation. Implementing these sections can be done iteratively if that's easier and we would be more than happy to help review. Some of the other sections may require additional work before implementation in the evaluator:

• 12.1 PartiQL ORDER BY syntax - NULLS FIRST, NULLS LAST, PRESERVE not yet added to the parser (though should be straightforward to implement)
• 12.3 Dependency of ORDER BY semantics on the Presence of Set Operators - not yet defined in the spec
• 12.4 SQL Compatibility ORDER BY clauses - CURRENT variable not yet implemented
• 12.6 Coercion of literals for SQL compatibility - implicit coercion of literals hasn't been implemented yet

We'd appreciate any contribution you or others could make!

[onurkuruu]

Hi, we have tried to implement ORDER BY by defined specifications. This implementation supports null/missing, bool, number, date/timestamp/time, string as defined at specifications but lob, set, struct are not implemented yet. This commit contains;

• Support for NULLS FIRST, NULLS LAST keywords
• Support for ordering null/missing, bool, number, date/timestamp/time, string types
• A bug fixed which SqlParser cannot parse if the statement contains ORDER BY which comes after GROUP BY
• Support for ordering the results;
  • Normal results
  • Aggregated results without grouping (GROUP BY)
  • Grouped results

Some specifications are not defined yet(12.3, 12.6) and still need to implement some other keywords like PRESERVE and CURRENT but we thought it can be the start of the implementation. We will add more test cases for this implementation. What do you think about this approach? (Related implementation link)

[alancai98]

@onurkuruu Thanks for the contribution! We'll start taking a look. It may be easier for us to provide feedback through a PR. If you don't mind, could you create a PR with your initial implementation?

[brack]

hello @raganhan @abbashus @mustafaakin -- I'm a Product Manager with Amazon Quantum Ledger Database (QLDB) collaborating with the PartiQL team. I was wondering if you'd be interested in chatting with us about how you're using PartiQL and capabilities like ORDER BY and GROUP BY. I couldn't find an email for you in GitHub so I figured I'd tag here quickly.

I also figured I'd check and see if the PR from @onurkuruu addressed your concerns and whether you had any additional questions.

If you'd be open to connecting, you can find my email on my GitHub profile. Cheers

[alancai98]

Closed by #554.