Hi Mihai,
Thank you for the reply and the pointers to those existing issues. I think the
test failures I see when attempting different approaches to fixing the
behaviour are a combination of bad tests — tests that just test the output of
the current implementation, even if this is incorrect — and other parts of the
code that make similar assumptions about scale that need to be changed
alongside any change to the SQL to Rex conversion.
Looking at the use of long data type to store the intervals, I wonder if this
would still be sufficient for day intervals to (the stated) microsecond
precision. I think that a signed long value should allow an interval of over
106 million days. Even the maximum fractional second precision of 9
(nanoseconds) would allow an interval of 106 thousand days to be represented. A
decimal would certainly be more flexible but maybe a long is sufficient here,
if it is a less disruptive change to the implementation?
My gut feeling is that the best approach for me to take would be to have these
intervals stored as microseconds, as indicated by their current default scale
and fractional second precision. Given the number of tests and other parts of
the code that are likely to need to be reworked alongside this change, I am
wary of attempting anything without some prior agreement from the maintainers
on the right approach to take.
Regards,
Mark.
________________________________
From: Mihai Budiu <[email protected]>
Sent: 28 February 2025 18:10
To: [email protected] <[email protected]>
Subject: Re: Question about scale after parsing day/time intervals to RexLiteral
Indeed, the "Default" precision of 3 seems to be hardwired in several places in
Calcite for TIMESTAMPs and INTERVALs (and maybe for TIME intervals too).
I have filed a related issue about the JavaTypeFactory:
https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fissues.apache.org%2Fjira%2Fbrowse%2FCALCITE-6752&data=05%7C02%7C%7Cb3b2a84b4d8e447683fd08dd5823441f%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638763630672738402%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=qrHXwDlyGf99r0ZKrBqFmpFMltZH4flhC1w0nj3E%2FZo%3D&reserved=0<https://issues.apache.org/jira/browse/CALCITE-6752>
See a list of related issues in the discussion for this other issue I have
filed.
https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fissues.apache.org%2Fjira%2Fbrowse%2FCALCITE-5919&data=05%7C02%7C%7Cb3b2a84b4d8e447683fd08dd5823441f%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638763630672761579%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=c6y8DtJN85gstZ3okmO6aoQab%2F1VdArRtcj%2B8Idc3Z0%3D&reserved=0<https://issues.apache.org/jira/browse/CALCITE-5919>
Fixing these end-to-end may be difficult.
Fixing the literals may be the easy part, but many other layers may need work,
even if you get the literals to represent the values you want. As I discuss in
the second issue, probably using BigDecimal is the right approach.
When you say it "breaks many tests", are the tests wrong? Then they should
break.
Mihai
________________________________
From: Mark Lewis <[email protected]>
Sent: Friday, February 28, 2025 9:40 AM
To: [email protected] <[email protected]>
Subject: Question about scale after parsing day/time intervals to RexLiteral
Using Calcite to parse an SQL interval day seems to always result in a
RexLiteral with a scale of 6, but with the value a millisecond representation
of the interval. This is because SqlLiteral.getValueAs() always returns
milliseconds, and the default fractional second precision is 6. The resulting
RexLiteral scale confused me since the value of 6 suggested a microsecond
representation.
I tried to change this behaviour in several ways:
1.
Change the default fractional second interval precision to 3.
2.
Change SqlNodeToRexConverterImpl.convertLiteral() to scale the value passed to
RexBuilder.makeIntervalLiteral() to microseconds in order to match the target
scale returned by
sqlIntervalQualifier.getFractionalSecondPrecision(rexBuilder.getTypeFactory().getTypeSystem()).
3.
Change SqlNodeToRexConverterImpl.convertLiteral() so that it creates a new
SqlIntervalQualifier that matches the one associated with the SqlLiteral but
with a fractional second precision of 3 to indicate milliseconds. This has no
effect because RexBuilder.makeIntervalLiteral() creates the RexLiteral's
RelDataType by passing the interval qualifier to
SqlTypeFactoryImpl.createSqlIntervalType(), and the canonize() call that
happens there discards the fractional second precision — I guess by using an
interned value with default fractional second precision.
While some of these approaches did produce a RexLiteral that looked more as I
expected, they all broke many Calcite tests.
Is it expected that day/time intervals should always be represented in
milliseconds but also report a scale of 6? If not, can you suggest the right
approach to implement the correct behaviour?
Thanks in advance for your help,
Mark.
