mihaibudiu commented on code in PR #4933:
URL: https://github.com/apache/calcite/pull/4933#discussion_r3360115381
##########
core/src/main/java/org/apache/calcite/runtime/CalciteResource.java:
##########
@@ -505,6 +505,15 @@ ExInst<SqlValidatorException>
intervalFieldExceedsPrecision(Number a0,
@BaseMessage("QUALIFY expression ''{0}'' must contain a window function")
ExInst<SqlValidatorException>
qualifyExpressionMustContainWindowFunction(String a0);
+ @BaseMessage("SELECT DISTINCT ON is not supported under the current SQL
conformance level")
+ ExInst<SqlValidatorException> distinctOnNotAllowed();
+
+ @BaseMessage("SELECT DISTINCT ON requires an ORDER BY clause")
+ ExInst<SqlValidatorException> distinctOnRequiresOrderBy();
+
+ @BaseMessage("SELECT DISTINCT ON expressions must match initial ORDER BY
expressions")
Review Comment:
what does "initial" mean here?
##########
core/src/main/java/org/apache/calcite/sql/SqlSelectOperator.java:
##########
@@ -37,14 +37,19 @@
* <p>Operands are:
*
* <ul>
- * <li>0: distinct ({@link SqlLiteral})</li>
+ * <li>0: keywordList ({@link SqlNodeList})</li>
Review Comment:
looks like there were some bugs in this comment...
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -3261,8 +3069,10 @@ private void registerQuery(
if (orderList != null) {
// If the query is 'SELECT DISTINCT', restrict the columns
// available to the ORDER BY clause.
+ // DISTINCT ON is an exception: ORDER BY may reference columns
+ // not in the SELECT list.
final SqlValidatorScope selectScope3 =
- select.isDistinct()
+ (select.isDistinct() && !select.isDistinctOn())
Review Comment:
So for distinctOn() isDistinct is true as well?
Or you can have SELECT DISTINCT DISTINCT ON(...)?
If yes, I hope there is a test
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -720,6 +720,9 @@ private static RelCollation requiredCollation(RelNode r) {
if (r instanceof Project) {
return requiredCollation(((Project) r).getInput());
}
+ if (r instanceof Filter) {
Review Comment:
Is this also a bug?
Seems unrelated to this PR.
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6627,7 +6507,122 @@ void testReturnsCorrectRowTypeOnCombinedJoin() {
f.withSql(qualifyOnMultipleWindowFunctions).ok();
}
- /** Negative tests for the {@code QUALIFY} clause. */
+ /** Test case of
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-5406";>[CALCITE-5406]
+ * Support the SELECT DISTINCT ON statement for PostgreSQL dialect</a>. */
+ @Test void testDistinctOnNotAllowed() {
+ // DISTINCT ON is not allowed under default SQL conformance
+ sql("^SELECT DISTINCT ON (deptno) empno FROM emp^ ORDER BY deptno")
+ .fails("SELECT DISTINCT ON is not supported under the current SQL
conformance level");
+ }
+
+ /** Test case of
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-5406";>[CALCITE-5406]
+ * Support the SELECT DISTINCT ON statement for PostgreSQL dialect</a>. */
+ @Test void testDistinctOnPositive() {
+ final SqlValidatorFixture f =
+ fixture().withConformance(SqlConformanceEnum.LENIENT);
+
+ f.withSql("SELECT DISTINCT ON (deptno) empno, ename FROM emp ORDER BY
deptno, empno")
+ .ok();
+
+ f.withSql("SELECT DISTINCT ON (deptno, job) empno FROM emp ORDER BY
deptno, job, hiredate")
+ .ok();
+
+ f.withSql("SELECT DISTINCT ON (deptno) empno FROM emp ORDER BY deptno
DESC")
+ .ok();
+
+ f.withSql("SELECT DISTINCT ON (deptno) empno FROM emp ORDER BY deptno
NULLS FIRST")
+ .ok();
+ }
+
+ /** Test case of
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-5406";>[CALCITE-5406]
+ * Support the SELECT DISTINCT ON statement for PostgreSQL dialect</a>. */
+ @Test void testDistinctOnNegative() {
+ final SqlValidatorFixture f =
+ fixture().withConformance(SqlConformanceEnum.LENIENT);
+
+ // DISTINCT ON requires ORDER BY
+ f.withSql("^SELECT DISTINCT ON (deptno) empno FROM emp^")
+ .fails("SELECT DISTINCT ON requires an ORDER BY clause");
+
+ // ORDER BY must contain all DISTINCT ON expressions as prefix
+ f.withSql("SELECT DISTINCT ON (deptno, job) empno FROM emp ORDER BY
^deptno^")
+ .fails("SELECT DISTINCT ON expressions must match initial ORDER BY
expressions");
+
+ // ORDER BY prefix must match exactly
+ f.withSql("SELECT DISTINCT ON (deptno, job) empno FROM emp ORDER BY ^job^,
deptno")
+ .fails("SELECT DISTINCT ON expressions must match initial ORDER BY
expressions");
+
+ // DISTINCT ON with extra ORDER BY is ok
+ f.withSql("SELECT DISTINCT ON (deptno) empno FROM emp ORDER BY deptno,
job")
+ .ok();
+
+ // Duplicate expressions in DISTINCT ON are allowed but must be matched in
ORDER BY
+ f.withSql("SELECT DISTINCT ON (deptno, deptno) deptno, empno FROM emp
ORDER BY deptno, deptno")
+ .ok();
+
+ // DISTINCT ON with expression
+ f.withSql("SELECT DISTINCT ON (empno % 2) empno, ename FROM emp ORDER BY
empno % 2")
+ .ok();
+
+ // Empty DISTINCT ON is not allowed (parse error)
+ f.withSql("SELECT DISTINCT ON ((^)^) deptno FROM emp")
+ .fails("(?s)Encountered \"\\)\" at .*");
+
+ // DISTINCT ON can reference alias (like ORDER BY)
+ f.withSql("SELECT DISTINCT ON (x) empno AS x, deptno FROM emp ORDER BY x")
+ .ok();
+
+ // DISTINCT ON with alias-column name clash: alias in SELECT takes
precedence
+ f.withSql("SELECT DISTINCT ON (deptno) empno AS deptno, deptno AS d FROM
emp ORDER BY deptno")
+ .ok();
+
+ // DISTINCT ON with qualified column reference
+ f.withSql("SELECT DISTINCT ON (e.deptno) e.deptno AS x, e.empno "
Review Comment:
Does this work if the ORDER BY column is not qualified?
Maybe there is such a test already
##########
babel/src/main/codegen/config.fmpp:
##########
@@ -619,6 +619,7 @@ data: {
includeIntervalWithoutQualifier: true
includeStarExclude: true
includeSelectBy: true
+ includeDistinctOn: true
Review Comment:
I think you need to edit the reference.md grammar too, if it's possible to
show babel-only extensions
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -3261,8 +3069,10 @@ private void registerQuery(
if (orderList != null) {
// If the query is 'SELECT DISTINCT', restrict the columns
// available to the ORDER BY clause.
+ // DISTINCT ON is an exception: ORDER BY may reference columns
+ // not in the SELECT list.
final SqlValidatorScope selectScope3 =
- select.isDistinct()
+ (select.isDistinct() && !select.isDistinctOn())
Review Comment:
And then you won't need an aggregate because the result has only 1 row?
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -813,12 +816,18 @@ protected void convertSelectImpl(
select.getQualify());
if (select.isDistinct()) {
- distinctify(bb, true);
+ if (!select.isDistinctOn()) {
Review Comment:
apparently it's impossible to have both DISTINCT and DISTINCT ON, so perhaps
isDistinct() should return 'false' for DISTINCT ON
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
