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

• Type: Bug
• Status: Closed
• Priority: Minor
• Resolution: Fixed
• Affects Version/s: 1.36.0
• Fix Version/s: 1.37.0
• Component/s: core
• Labels: pull-request-available
• Assignee: Unassigned
• Reporter: Bruno Volpato
• Created: 06/Apr/24 02:26
• Updated: 06/May/24 22:19
• Resolved: 10/Apr/24 19:51

Description

 

We are using RelToSqlConverter, and seeing issues with it generating invalid queries when using DESC NULLS LAST, specifically.

 

For example, this test query:

 

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

Gets resolved correctly, with a subquery, to:

 

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) AS "t3" 

 

 

However, if I specify `desc nulls last`:

 

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

It creates an invalid query (order by 2, but only one field was projected):

 

 

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

 

 

 

Trying to troubleshoot it, it appears that without the `NULLS LAST`, we have the following instance:

 

SqlBasicCall -> SqlNumericLiteral 

 

 

But when including it, it gets wrapped in another call:

 

SqlBasicCall -> SqlBasicCall -> SqlNumericLiteral 

 

 

So the hasSortByOrdinal method ends up returning false, which causes `needNewSubQuery` to incorrectly report false too.

 

It appears that the best way to deal with this is by using a recursion to find numeric literals - but let me know if there are better ideas. 

 

I plan to take a stab at this since I got enough context.

 

 

Issue Links

• links to: GitHub Pull Request #3754