IGNITE-28896 SQL Calcite: Support BigDecimal for FETCH, OFFSET and LIMIT - #13375
IGNITE-28896 SQL Calcite: Support BigDecimal for FETCH, OFFSET and LIMIT#13375tkalkirill wants to merge 9 commits into
Conversation
tkalkirill
changed the title
|
So, it need to be merged somehow ? #13311 |
|
@zstan BigDecimal is already in Calcite's main branch, so I think it makes sense to use that. |
| rowType, | ||
| idxBndRel.first() ? cmp : cmp.reversed(), | ||
| 0, | ||
| SortNode.OFFSET_DEFAULT, |
There was a problem hiding this comment.
I specially not touch this in my PR ) What more informative ?
node with OFFSET_DEFAULT param - it need to lookup for reviewer or node with concreete "0" param ?
Of course it`s all kinda taste ... I just show you my logic here
There was a problem hiding this comment.
I see your point. My intention is to express the semantics of this positional argument: no offset is specified here. The reader does not need to look up the constant value unless implementation details matter, while 0 shows the value but not the intent. It also keeps this call consistent with the defaults defined by SortNode, so I’d prefer to keep it.
| else { | ||
| SqlTypeName paramType = typeFactory().createType(param.getClass()).getSqlTypeName(); | ||
| err = IgniteResource.INSTANCE.incorrectDynamicParameterType(SqlTypeName.BIGINT.toString(), paramType.getName()); | ||
| err = IgniteResource.INSTANCE.incorrectDynamicParameterType(SqlTypeName.DECIMAL.toString(), paramType.getName()); |
There was a problem hiding this comment.
say once more: this is wrong fix !
There was a problem hiding this comment.
Why? That’s how it is in the CALCITE main branch right now.
| public void testFetchOffsetRoundingPolicy() throws Exception { | ||
| fillCache(cacheRepl, 4); | ||
|
|
||
| FetchOffsetRoundingPolicy floorPlc = value -> value.setScale(0, RoundingMode.FLOOR); |
There was a problem hiding this comment.
I think we do not need to test Calcite configuration setting, we need to fix it in our code and use over all functionality and tests, otherwize we need to give an ability to configure it through Ignite setting which is discussable.
There was a problem hiding this comment.
I don’t think this needs to become a public Ignite setting. Rounding semantics may depend on the required SQL compatibility mode, so having one hardcoded behavior for every use case is unnecessarily restrictive.
This policy is an internal Calcite extension point intended for advanced integrations, not a part of the public Ignite API. The default behavior is still used everywhere unless a custom policy is explicitly provided. Keeping it internal also allows us to evolve or replace the mechanism later without introducing a public API compatibility burden.
2 participants
https://issues.apache.org/jira/browse/IGNITE-28896