-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[CALCITE-7624] Support BigDecimal for FETCH and OFFSET in Enumerable #5050
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 7 commits
2450ecd
141cfac
2b3ffa3
e289003
be1922d
2eec070
9c27c74
80ed8c4
54de763
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,36 +101,50 @@ public static EnumerableLimit create(final RelNode input, @Nullable RexNode offs | |
| result.format); | ||
|
|
||
| Expression v = builder.append("child", result.block); | ||
| Expression roundingPolicyExp = getRoundingPolicy(implementor); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need such a complex strategy; we can control the rounding method using expressions instead. We simply need to select a standard approach and document it with a comment. I suspect @mihaibudiu would also prefer a simpler implementation.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this approach, users can quite easily override this behavior.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't your previous PR add support for expressions(like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Every flag like this increases the space of configurations to test. I would keep this one simple by hardwiring the rounding policy.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @xiedeyantu And how can we override the expression later if necessary?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I really meant was that if we could support expressions in
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @xiedeyantu I haven't seen any objections to |
||
| if (offset != null) { | ||
| v = | ||
| builder.append("offset", | ||
| Expressions.call(v, BuiltInMethod.SKIP.method, | ||
| getExpression(offset))); | ||
| Expressions.call(BuiltInMethod.SKIP_BIG_DECIMAL.method, v, | ||
| getExpression(offset, "OFFSET", roundingPolicyExp))); | ||
| } | ||
| if (fetch != null) { | ||
| v = | ||
| builder.append("fetch", | ||
| Expressions.call(v, BuiltInMethod.TAKE.method, | ||
| getExpression(fetch))); | ||
| Expressions.call(BuiltInMethod.TAKE_BIG_DECIMAL.method, v, | ||
| getExpression(fetch, "FETCH", roundingPolicyExp))); | ||
| } | ||
|
|
||
| builder.add(Expressions.return_(null, v)); | ||
| return implementor.result(physType, builder.toBlock()); | ||
| } | ||
|
|
||
| static Expression getExpression(RexNode rexNode) { | ||
| static Expression getExpression(RexNode rexNode, String kind, | ||
| Expression roundingPolicy) { | ||
| final Expression value; | ||
| if (rexNode instanceof RexDynamicParam) { | ||
| final RexDynamicParam param = (RexDynamicParam) rexNode; | ||
| return Expressions.convert_( | ||
| value = | ||
| Expressions.call(DataContext.ROOT, | ||
| BuiltInMethod.DATA_CONTEXT_GET.method, | ||
| Expressions.constant("?" + param.getIndex())), | ||
| Integer.class); | ||
| Expressions.constant("?" + param.getIndex())); | ||
| } else { | ||
| // TODO: Enumerable runtime only supports INT types for FETCH and OFFSET, not BIGINT types. | ||
| // Currently, using BIGINT types for execution will result in an error message. | ||
| // This issue needs to be fixed. For more information, see CALCITE-7156. | ||
| return Expressions.constant(RexLiteral.intValue(rexNode)); | ||
| value = Expressions.constant(RexLiteral.bigDecimalValue(rexNode)); | ||
| } | ||
| return Expressions.call( | ||
| BuiltInMethod.NUMBER_TO_BIG_DECIMAL_LIMIT.method, | ||
| value, | ||
| Expressions.constant(kind), | ||
| roundingPolicy); | ||
| } | ||
|
|
||
| static Expression getRoundingPolicy(EnumerableRelImplementor implementor) { | ||
| final Object roundingPolicy = | ||
| implementor.map.get(EnumerableRelImplementor.FETCH_OFFSET_ROUNDING_POLICY); | ||
| if (roundingPolicy == null) { | ||
| return Expressions.field(null, FetchOffsetRoundingPolicy.class, "NONE"); | ||
| } | ||
| return implementor.stash((FetchOffsetRoundingPolicy) roundingPolicy, | ||
| FetchOffsetRoundingPolicy.class); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to you under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.calcite.adapter.enumerable; | ||
|
|
||
| import java.math.BigDecimal; | ||
|
|
||
| /** Defines how enumerable FETCH and OFFSET values are rounded. */ | ||
| public interface FetchOffsetRoundingPolicy { | ||
| /** Default policy that preserves the value produced by validation or | ||
| * parameter binding. */ | ||
| FetchOffsetRoundingPolicy NONE = value -> value; | ||
|
|
||
| /** Rounds a FETCH or OFFSET value. */ | ||
| BigDecimal round(BigDecimal value); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain the purpose of adding "Limit" to the name of the method "numberToBigDecimalLimit"? Or could you simply name it "numberToBigDecimal" and explain what the method does using the Javadoc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed and updated the documentation.