Skip to content

IGNITE-28801 Calcite. Bump calcite dependency version to 1.42#13274

Open
zstan wants to merge 10 commits into
apache:masterfrom
zstan:ignite-28801
Open

IGNITE-28801 Calcite. Bump calcite dependency version to 1.42#13274
zstan wants to merge 10 commits into
apache:masterfrom
zstan:ignite-28801

Conversation

@zstan

@zstan zstan commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

BuiltInMethod.BIG_DECIMAL_NEGATE.getMethodName());
defineUnary(UNARY_PLUS, UnaryPlus, NullPolicy.STRICT, null);
// checked arithmetic
defineBinary(CHECKED_PLUS, AddChecked, NullPolicy.STRICT, "checkedPlus");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

due to categorical CHECED type without conformance flag check
https://issues.apache.org/jira/browse/CALCITE-7443

// For checked arithmetic call the method.
if (CHECKED_OPERATORS.contains(op))
return Expressions.call(SqlFunctions.class, backupMethodName, argValueList);
//if (CHECKED_OPERATORS.contains(op))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public static Expression makeBinary(ExpressionType binaryType, Expression left, Expression right) {
switch (binaryType) {
case Add:
case Add, AddChecked:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConstantExpression formatExpr;

// Check for FORMAT clause if second operand is available in RexCall.
if (call.operandCount() == 2) {

@zstan zstan Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just runtime code alignment

"BITXOR",
SqlKind.OTHER_FUNCTION,
ReturnTypes.LEAST_RESTRICTIVE,
ReturnTypes.BIGINT_NULLABLE,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after this
https://issues.apache.org/jira/browse/CALCITE-6731
return type resolved as Object which breaks futrher scalar compilation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks weird. Can we write new own SqlReturnTypeInference to derive correct data type as workaround?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plz explain ? now it can return nullable bigint and unsigned but unsigned is not supported, you mean we can loose perf with unsigned ?

/** {@inheritDoc} */
@Override public int getMaxNumericScale() {
return Short.MAX_VALUE;
@Override public int getMaxScale(SqlTypeName typeName) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getMaxNumericScale\getMaxNumericPrecision - deprecated and moved to final notation

@zstan zstan force-pushed the ignite-28801 branch 3 times, most recently from 35bc26e to bf6ecc6 Compare June 26, 2026 14:23
Comment thread checkstyle/checkstyle-suppressions.xml Outdated
files="ConcurrentLinkedHashMap\.java"/>

<suppress checks=".*"
files="RexImpTable\.java|RexToLixTranslator\.java"/>

@zstan zstan Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let`s remove checks from calcite runtime files, it`s hard to compare with base branch otherwize

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently only limited lines have been added, but already error (unused import) was introduced. I think we should check these files too, at least for now.

import org.apache.calcite.rex.RexCall;
import org.apache.calcite.rex.RexLiteral;
import org.apache.calcite.rex.RexNode;
import org.apache.calcite.runtime.FlatLists;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import.
We should not exclude this file from checkstyle to prevent such errors

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disagree, we need to align all runtime but not in this PR, thus - skip this minors for now

final RexToLixTranslator translator,
final RexCall call,
final List<Expression> argValueList) {
List<Expression> argValueList) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it still final, why final was removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it`s not a final on apache calcite

Comment on lines +1361 to +1362
} else if (expressionType == NegateChecked && null != backupMethodName) {
e = Expressions.call(SqlFunctions.class, backupMethodName, argValueList);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep codestyle and just add these two lines

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to have a possibility easily compare current and our runtime implementations - for such a case we need disable checkstyle for whole file


if (!allowNulls.get(p)) {
collsAllowNullsBuilder.clear(c);
if (!nullExclusions.get(p)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nullExclusions and allowNulls have inverse logic, why here not allowNulls changed to not null exclusion? Shouldn't we remove not?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, i understand what you mean, "not" is changed in comparator implementation side, or if you about namings - let`s discuss it

"BITXOR",
SqlKind.OTHER_FUNCTION,
ReturnTypes.LEAST_RESTRICTIVE,
ReturnTypes.BIGINT_NULLABLE,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks weird. Can we write new own SqlReturnTypeInference to derive correct data type as workaround?

* mvn generate-sources -pl :ignite-calcite
*/

package org.apache.ignite.internal.processors.query.calcite.sql.generated;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it was removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can`t restore why it been removed, what purpose of this file ?

.and(s -> "=($0, $cor0.ID)".equals(s.condition().toString()))
.and(hasChildThat(isInstanceOf(IgniteTableScan.class)
.and(t -> "<($t0, 50)".equals(t.condition().toString()))))));
.and(t -> "<(CAST($t0):INTEGER, 50)".equals(t.condition().toString()))))));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change field definition from Integer.class to SqlTypeName.INTEGER instead

Expression operand) {
Expression operand,
boolean safe,
ConstantExpression format) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we allow format now (have format processing code in CastImplementor) why do we ignore it here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I not to plan aling all runtime in this PR, let`s store such questions for align part ?

Comment thread checkstyle/checkstyle-suppressions.xml Outdated
files="ConcurrentLinkedHashMap\.java"/>

<suppress checks=".*"
files="RexImpTable\.java|RexToLixTranslator\.java"/>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently only limited lines have been added, but already error (unused import) was introduced. I think we should check these files too, at least for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants