[CALCITE-6355] RelToSqlConverter[ORDER BY] generates an incorrect order by when NULLS LAST is used in non-projected field

[bvolpato]

There are some issues in the needNewSubQuery -> hasSortByOrdinal method in SqlImplementor when using RelToSqlConverter specifying NULLS LAST, for some particular queries.

The method hasSortByOrdinal fails to recognize that the order might contain nested calls. For example, we can easily produce SqlSelect -> SqlNodeList order -> SqlBasicCall (nulls last) -> SqlBasicCall (desc) -> SqlNumericLiteral, but it can only match correctly if there's a single SqlBasicCall in the chain.

With that, this query:

select "product_id"
from "product"
where "net_weight" is not null
group by "product_id"
order by MAX("net_weight") desc nulls last 

Resolves to an invalid query. Notice the order by 2, however only one field was projected (product_id).

SELECT "product_id"
FROM "foodmart"."product"
WHERE "net_weight" IS NOT NULL
GROUP BY "product_id"
ORDER BY 2 DESC NULLS LAST 

With my fix, it produces the correct SELECT with subquery:

SELECT "product_id"
FROM (SELECT "product_id", MAX("net_weight") AS "EXPR$1"
FROM "foodmart"."product"
WHERE "net_weight" IS NOT NULL
GROUP BY "product_id"
ORDER BY 2 DESC NULLS LAST) AS "t3"

(I've also provided some context at https://issues.apache.org/jira/browse/CALCITE-6355).

[caicancai]

@bvolpato Calcite's jira message does not allow the Fix keyword to begin with

[bvolpato]

Attempted to fix the checker errors (added annotations & handled nullness). It hadn't failed ./gradlew build before, needed to explicitly -PenableCheckerframework.

[bvolpato]

This test is flaky / unrelated to my changes:

            Caused by: com.github.dockerjava.api.exception.DockerClientException: Could not pull image: [DEPRECATION NOTICE] Docker Image Format v1 and Docker Image manifest version 2, schema 1 support is disabled by default and will be removed in an upcoming release. Suggest the author of docker.io/library/redis:2.8.19 to upgrade the image to the OCI Format or Docker Image manifest v2, schema 2. More information at https://docs.docker.com/go/deprecated-image-specs/

                at app//com.github.dockerjava.api.command.PullImageResultCallback.checkDockerClientPullSuccessful(PullImageResultCallback.java:97)

                at app//com.github.dockerjava.api.command.PullImageResultCallback.throwFirstError(PullImageResultCallback.java:112)

                at app//com.github.dockerjava.api.async.ResultCallbackTemplate.awaitCompletion(ResultCallbackTemplate.java:93)

                at app//org.testcontainers.images.TimeLimitedLoggedPullImageResultCallback.awaitCompletion(TimeLimitedLoggedPullImageResultCallback.java:58)

                at app//org.testcontainers.images.RemoteDockerImage.resolve(RemoteDockerImage.java:97)

                ... 46 more



FAILURE  10.5sec,    1 completed,   1 failed,   0 skipped, org.apache.calcite.adapter.redis.RedisAdapterCaseBase

FAILURE  12.6sec,    2 completed,   1 failed,   1 skipped, Gradle Test Run :redis:test

[mihaibudiu]

Otherwise this looks pretty good.

[mihaibudiu]

So far we have required the JIRA case title, the PR case title, and the comments in the code to use the exact same text.
This makes it easy to understand how these are related.
Can you please follow these guidelines?
There are lots of examples.

[bvolpato]

Sorry, missed this. I've adjusted the title here -- thank you. Do I have to reword commits? Or the title of the PR will be used when merged / squashed?

[mihaibudiu]

Eventually after the PR is accepted you will have to squash all the commits to just one, and that one will have to have the exact same message as the JIRA title, and this PR title. Moreover, the JavaDoc for the test cases related to the fix also have to have a relatively rigid format, providing the link to the jira case and the full text in the jira title.

[mihaibudiu]

This looks fine to me, but I am not very familiar with this part of Calcite.
If anyone else who knows this better could take a look I would feel more confident.
The test shows that this is doing something good, though.

[mihaibudiu]

To merge this you will have to squash the commits into a single commit, and the commit message has to be exactly the JIRA title. Take a look at the other merged commits into main for examples.

[bvolpato]

To merge this you will have to squash the commits into a single commit, and the commit message has to be exactly the JIRA title. Take a look at the other merged commits into main for examples.

Done, thanks for your help @mihaibudiu!

[sonarcloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud