[
https://issues.apache.org/jira/browse/DRILL-7307?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16879141#comment-16879141
]
ASF GitHub Bot commented on DRILL-7307:
---------------------------------------
dgrinchenko commented on pull request #1820: DRILL-7307: casthigh for decimal
type can lead to the issues with VarDecimalHolder
URL: https://github.com/apache/drill/pull/1820#discussion_r300637551
##########
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/expr/EvaluationVisitor.java
##########
@@ -1126,327 +1131,159 @@ public HoldingContainer
visitFunctionCall(FunctionCall e, ClassGenerator<?> gene
+ "It should have been converted to FunctionHolderExpression in
materialization");
}
- @Override
- public HoldingContainer
visitFunctionHolderExpression(FunctionHolderExpression e, ClassGenerator<?>
generator)
+ private <T> HoldingContainer visitExpression(T e, ClassGenerator<?>
generator, GetHolderCall getHolder)
throws RuntimeException {
if (constantBoundaries.contains(e)) {
generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitFunctionHolderExpression(e, generator);
- // generator.getMappingSet().exitConstant();
- // return c;
- return renderConstantExpression(generator, c);
+ HoldingContainer c = getHolder.call();
+
+ return renderConstantExpression(generator, c, e);
} else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitFunctionHolderExpression(e,
generator).setConstant(true);
+ return getHolder.call().setConstant(true);
} else {
- return super.visitFunctionHolderExpression(e, generator);
+ return getHolder.call();
}
}
+ @Override
+ public HoldingContainer
visitFunctionHolderExpression(FunctionHolderExpression e, ClassGenerator<?>
generator)
+ throws RuntimeException {
+ return visitExpression(e, generator, () ->
super.visitFunctionHolderExpression(e, generator));
+ }
+
@Override
public HoldingContainer visitBooleanOperator(BooleanOperator e,
ClassGenerator<?> generator)
throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitBooleanOperator(e, generator);
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitBooleanOperator(e, generator).setConstant(true);
- } else {
- return super.visitBooleanOperator(e, generator);
- }
+ return visitExpression(e, generator, () -> super.visitBooleanOperator(e,
generator));
}
@Override
public HoldingContainer visitIfExpression(IfExpression e,
ClassGenerator<?> generator) throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitIfExpression(e, generator);
- // generator.getMappingSet().exitConstant();
- // return c;
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitIfExpression(e, generator).setConstant(true);
- } else {
- return super.visitIfExpression(e, generator);
- }
+ return visitExpression(e, generator, () -> super.visitIfExpression(e,
generator));
}
@Override
public HoldingContainer visitSchemaPath(SchemaPath e, ClassGenerator<?>
generator) throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitSchemaPath(e, generator);
- // generator.getMappingSet().exitConstant();
- // return c;
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitSchemaPath(e, generator).setConstant(true);
- } else {
- return super.visitSchemaPath(e, generator);
- }
+ return visitExpression(e, generator, () -> super.visitSchemaPath(e,
generator));
}
@Override
public HoldingContainer visitLongConstant(LongExpression e,
ClassGenerator<?> generator) throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitLongConstant(e, generator);
- // generator.getMappingSet().exitConstant();
- // return c;
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitLongConstant(e, generator).setConstant(true);
- } else {
- return super.visitLongConstant(e, generator);
- }
+ return visitExpression(e, generator, () -> super.visitLongConstant(e,
generator));
}
@Override
public HoldingContainer visitDecimal9Constant(Decimal9Expression e,
ClassGenerator<?> generator)
throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitDecimal9Constant(e, generator);
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitDecimal9Constant(e, generator).setConstant(true);
- } else {
- return super.visitDecimal9Constant(e, generator);
- }
+ return visitExpression(e, generator, () ->
super.visitDecimal9Constant(e, generator));
}
@Override
public HoldingContainer visitDecimal18Constant(Decimal18Expression e,
ClassGenerator<?> generator)
throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitDecimal18Constant(e, generator);
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitDecimal18Constant(e, generator).setConstant(true);
- } else {
- return super.visitDecimal18Constant(e, generator);
- }
+ return visitExpression(e, generator, () ->
super.visitDecimal18Constant(e, generator));
}
@Override
public HoldingContainer visitDecimal28Constant(Decimal28Expression e,
ClassGenerator<?> generator)
throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitDecimal28Constant(e, generator);
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitDecimal28Constant(e, generator).setConstant(true);
- } else {
- return super.visitDecimal28Constant(e, generator);
- }
+ return visitExpression(e, generator, () ->
super.visitDecimal28Constant(e, generator));
}
@Override
public HoldingContainer visitDecimal38Constant(Decimal38Expression e,
ClassGenerator<?> generator)
throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitDecimal38Constant(e, generator);
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitDecimal38Constant(e, generator).setConstant(true);
- } else {
- return super.visitDecimal38Constant(e, generator);
- }
+ return visitExpression(e, generator, () ->
super.visitDecimal38Constant(e, generator));
}
@Override
public HoldingContainer visitVarDecimalConstant(VarDecimalExpression e,
ClassGenerator<?> generator)
throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitVarDecimalConstant(e, generator);
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitVarDecimalConstant(e, generator).setConstant(true);
- } else {
- return super.visitVarDecimalConstant(e, generator);
- }
+ return visitExpression(e, generator, () ->
super.visitVarDecimalConstant(e, generator));
}
@Override
public HoldingContainer visitIntConstant(IntExpression e,
ClassGenerator<?> generator) throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitIntConstant(e, generator);
- // generator.getMappingSet().exitConstant();
- // return c;
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitIntConstant(e, generator).setConstant(true);
- } else {
- return super.visitIntConstant(e, generator);
- }
+ return visitExpression(e, generator, () -> super.visitIntConstant(e,
generator));
}
@Override
public HoldingContainer visitDateConstant(DateExpression e,
ClassGenerator<?> generator) throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitDateConstant(e, generator);
-
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitDateConstant(e, generator).setConstant(true);
- } else {
- return super.visitDateConstant(e, generator);
- }
+ return visitExpression(e, generator, () -> super.visitDateConstant(e,
generator));
}
@Override
public HoldingContainer visitTimeConstant(TimeExpression e,
ClassGenerator<?> generator) throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitTimeConstant(e, generator);
-
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitTimeConstant(e, generator).setConstant(true);
- } else {
- return super.visitTimeConstant(e, generator);
- }
+ return visitExpression(e, generator, () -> super.visitTimeConstant(e,
generator));
}
@Override
public HoldingContainer visitIntervalYearConstant(IntervalYearExpression
e, ClassGenerator<?> generator)
throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitIntervalYearConstant(e, generator);
-
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitIntervalYearConstant(e, generator).setConstant(true);
- } else {
- return super.visitIntervalYearConstant(e, generator);
- }
+ return visitExpression(e, generator, () ->
super.visitIntervalYearConstant(e, generator));
}
@Override
public HoldingContainer visitTimeStampConstant(TimeStampExpression e,
ClassGenerator<?> generator)
throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitTimeStampConstant(e, generator);
-
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitTimeStampConstant(e, generator).setConstant(true);
- } else {
- return super.visitTimeStampConstant(e, generator);
- }
+ return visitExpression(e, generator, () ->
super.visitTimeStampConstant(e, generator));
}
@Override
public HoldingContainer visitFloatConstant(FloatExpression e,
ClassGenerator<?> generator)
throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitFloatConstant(e, generator);
- // generator.getMappingSet().exitConstant();
- // return c;
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitFloatConstant(e, generator).setConstant(true);
- } else {
- return super.visitFloatConstant(e, generator);
- }
+ return visitExpression(e, generator, () -> super.visitFloatConstant(e,
generator));
}
@Override
public HoldingContainer visitDoubleConstant(DoubleExpression e,
ClassGenerator<?> generator)
throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitDoubleConstant(e, generator);
- // generator.getMappingSet().exitConstant();
- // return c;
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitDoubleConstant(e, generator).setConstant(true);
- } else {
- return super.visitDoubleConstant(e, generator);
- }
+ return visitExpression(e, generator, () -> super.visitDoubleConstant(e,
generator));
}
@Override
public HoldingContainer visitBooleanConstant(BooleanExpression e,
ClassGenerator<?> generator)
throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitBooleanConstant(e, generator);
- // generator.getMappingSet().exitConstant();
- // return c;
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitBooleanConstant(e, generator).setConstant(true);
- } else {
- return super.visitBooleanConstant(e, generator);
- }
+ return visitExpression(e, generator, () -> super.visitBooleanConstant(e,
generator));
}
@Override
public HoldingContainer visitUnknown(LogicalExpression e,
ClassGenerator<?> generator) throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitUnknown(e, generator);
- // generator.getMappingSet().exitConstant();
- // return c;
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitUnknown(e, generator).setConstant(true);
- } else {
- return super.visitUnknown(e, generator);
- }
+ return visitExpression(e, generator, () -> super.visitUnknown(e,
generator));
}
@Override
public HoldingContainer visitQuotedStringConstant(QuotedString e,
ClassGenerator<?> generator)
throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitQuotedStringConstant(e, generator);
- // generator.getMappingSet().exitConstant();
- // return c;
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitQuotedStringConstant(e, generator).setConstant(true);
- } else {
- return super.visitQuotedStringConstant(e, generator);
- }
+ return visitExpression(e, generator, () ->
super.visitQuotedStringConstant(e, generator));
}
@Override
public HoldingContainer visitIntervalDayConstant(IntervalDayExpression e,
ClassGenerator<?> generator)
throws RuntimeException {
- if (constantBoundaries.contains(e)) {
- generator.getMappingSet().enterConstant();
- HoldingContainer c = super.visitIntervalDayConstant(e, generator);
-
- return renderConstantExpression(generator, c);
- } else if (generator.getMappingSet().isWithinConstant()) {
- return super.visitIntervalDayConstant(e, generator).setConstant(true);
- } else {
- return super.visitIntervalDayConstant(e, generator);
- }
+ return visitExpression(e, generator, () ->
super.visitIntervalDayConstant(e, generator));
}
/*
* Get a HoldingContainer for a constant expression. The returned
* HoldingContainder will indicate it's for a constant expression.
*/
- private HoldingContainer renderConstantExpression(ClassGenerator<?>
generator, HoldingContainer input) {
- JVar fieldValue = generator.declareClassField("constant",
generator.getHolderType(input.getMajorType()));
+ private <T> HoldingContainer renderConstantExpression(ClassGenerator<?>
generator, HoldingContainer input,
Review comment:
it makes no sense, as we need `DrillFuncHolderExpr` to get information about
null handling for the expression. The override logic is applied only for
NULL_IF_NULL functions. With the suggested changes, the only way would be to
override `mode` for each `FunctionHolderExpression` or do the same check inside
render for the `FunctionHolderExpression`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> casthigh for decimal type can lead to the issues with VarDecimalHolder
> ----------------------------------------------------------------------
>
> Key: DRILL-7307
> URL: https://issues.apache.org/jira/browse/DRILL-7307
> Project: Apache Drill
> Issue Type: Bug
> Reporter: Dmytriy Grinchenko
> Assignee: Dmytriy Grinchenko
> Priority: Critical
> Fix For: 1.17.0
>
>
> The decimal cast may lead to issues with VarDercimal transformation and
> issues at uml functions which using casthigh under the hood
> Example:
> {code}
> apache drill> select casthigh(cast(1025.0 as decimal(28,8)));
> Error: SYSTEM ERROR: CompileException: Line 25, Column 60: "isSet" is neither
> a method, a field, nor a member class of
> "org.apache.drill.exec.expr.holders.VarDecimalHolder"
> Fragment 0:0
> Please, refer to logs for more information.
> {code}
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
