Skip to content

Commit

Permalink
Reverting previous changes of a new DruidSqlValidator field and inste…
Browse files Browse the repository at this point in the history
…ad creating a custom SQLIdentifier class for the Time column
  • Loading branch information
gargvishesh committed Oct 10, 2023
1 parent b98ffcf commit 295d4a7
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 41 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.calcite.rex.RexBuilder;
import org.apache.calcite.rex.RexLiteral;
import org.apache.calcite.rex.RexNode;
import org.apache.calcite.runtime.CalciteContextException;
import org.apache.calcite.sql.SqlAggFunction;
import org.apache.calcite.sql.SqlCall;
import org.apache.calcite.sql.SqlFunctionCategory;
Expand All @@ -39,6 +40,7 @@
import org.apache.calcite.sql.type.SqlTypeName;
import org.apache.calcite.sql.type.SqlTypeUtil;
import org.apache.calcite.sql.validate.SqlValidator;
import org.apache.calcite.sql.validate.SqlValidatorException;
import org.apache.calcite.util.Optionality;
import org.apache.druid.error.DruidException;
import org.apache.druid.error.InvalidSqlInput;
Expand All @@ -64,7 +66,6 @@
import org.apache.druid.sql.calcite.expression.DruidExpression;
import org.apache.druid.sql.calcite.expression.Expressions;
import org.apache.druid.sql.calcite.planner.Calcites;
import org.apache.druid.sql.calcite.planner.DruidSqlValidator;
import org.apache.druid.sql.calcite.planner.PlannerContext;
import org.apache.druid.sql.calcite.rel.VirtualColumnRegistry;

Expand Down Expand Up @@ -326,6 +327,40 @@ public RelDataType inferReturnType(SqlOperatorBinding sqlOperatorBinding)
}
}

private static class TimeColIdentifer extends SqlIdentifier
{

public TimeColIdentifer()
{
super("__time", SqlParserPos.ZERO);
}

@Override
public <R> R accept(org.apache.calcite.sql.util.SqlVisitor<R> visitor)
{

try {
return super.accept(visitor);
}
catch (CalciteContextException e) {
if (e.getCause() instanceof SqlValidatorException) {
throw DruidException.forPersona(DruidException.Persona.ADMIN)
.ofCategory(DruidException.Category.INVALID_INPUT)
.build(
e,
"Query could not be planned. A possible reason is [%s]",
"LATEST and EARLIEST aggregators implicitly depend on the __time column, but the "
+ "table queried doesn't contain a __time column. Please use LATEST_BY or EARLIEST_BY "
+ "and specify the column explicitly."
);

} else {
throw e;
}
}
}
}

private static class EarliestLatestSqlAggFunction extends SqlAggFunction
{
private static final EarliestLatestReturnTypeInference EARLIEST_LATEST_ARG0_RETURN_TYPE_INFERENCE =
Expand Down Expand Up @@ -384,14 +419,12 @@ public SqlNode rewriteCall(

List<SqlNode> newOperands = new ArrayList<>();
newOperands.add(operands.get(0));
newOperands.add(new SqlIdentifier("__time", SqlParserPos.ZERO));
newOperands.add(new TimeColIdentifer());

if (operands.size() == 2) {
newOperands.add(operands.get(1));
}

((DruidSqlValidator) validator).setEarliestLatestByConverted();

return replacementAggFunc.createCall(pos, newOperands);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@
import org.apache.calcite.tools.RelBuilder;
import org.apache.calcite.tools.ValidationException;
import org.apache.calcite.util.Pair;
import org.apache.druid.error.DruidException;

import javax.annotation.Nullable;
import java.io.Reader;
Expand Down Expand Up @@ -241,27 +240,6 @@ public SqlNode validate(SqlNode sqlNode) throws ValidationException
validatedSqlNode = validator.validate(sqlNode);
}
catch (RuntimeException e) {
if (((DruidSqlValidator) validator).getEarliestLatestByConverted() && e.getCause()
.getMessage()
.contains("__time")){

// Since __time column may have been introduced by query rewrite from EARLIEST/LATEST to EARLIEST_BY/LATEST_BY,
// raise a custom exception informing the user of the implicit dependency. Pre-existence of __time col separately
// in query and being the cause of validation failure is possible, but the validation order between the
// new %_BY operator and existing __time col. is unclear, and may lead to confusing "col __time not found in
// any table (row x, col y)" error pointing to the rewritten operator.

throw DruidException.forPersona(DruidException.Persona.ADMIN)
.ofCategory(DruidException.Category.INVALID_INPUT)
.build(
e,
"Query could not be planned. A possible reason is [%s]",
"LATEST and EARLIEST aggregators implicitly depend on the __time column, but the "
+ "table queried doesn't contain a __time column. Please use LATEST_BY or EARLIEST_BY "
+ "and specify the column explicitly."
);

}
throw new ValidationException(e);
}
state = CalcitePlanner.State.STATE_4_VALIDATED;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,8 @@
* Druid extended SQL validator. (At present, it doesn't actually
* have any extensions yet, but it will soon.)
*/
public class DruidSqlValidator extends BaseDruidSqlValidator
class DruidSqlValidator extends BaseDruidSqlValidator
{

private Boolean earliestLatestByConverted;

protected DruidSqlValidator(
SqlOperatorTable opTab,
CalciteCatalogReader catalogReader,
Expand All @@ -41,16 +38,5 @@ protected DruidSqlValidator(
)
{
super(opTab, catalogReader, typeFactory, validatorConfig);
earliestLatestByConverted = false;
}

public Boolean getEarliestLatestByConverted()
{
return earliestLatestByConverted;
}

public void setEarliestLatestByConverted()
{
this.earliestLatestByConverted = true;
}
}

0 comments on commit 295d4a7

Please sign in to comment.