LakshSingla commented on code in PR #15920:
URL: https://github.com/apache/druid/pull/15920#discussion_r1493802233
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/sql/MSQTaskSqlEngine.java:
##########
@@ -307,6 +337,124 @@ private static void validateLimitAndOffset(final RelNode
topRel, final boolean l
}
}
+ /**
+ * Validate that the query does not include any type changes from string to
array or vice versa.
+ *
+ * These type changes tend to cause problems due to mixing of multi-value
strings and string arrays. In particular,
+ * many queries written in the "classic MVD" style (treating MVDs as if they
were regular strings) will fail when
+ * MVDs and arrays are mixed. So, we detect them as invalid.
+ *
+ * @param rootRel root rel
+ * @param fieldMappings field mappings from {@link #validateInsert(RelNode,
List, Table, PlannerContext)}
+ * @param targetTable table we are inserting (or replacing) into, if any
+ * @param plannerContext planner context
+ */
+ private static void validateTypeChanges(
+ final RelNode rootRel,
+ final List<Pair<Integer, String>> fieldMappings,
+ @Nullable final Table targetTable,
+ final PlannerContext plannerContext
+ )
+ {
+ if (targetTable == null) {
+ return;
+ }
+
+ final Set<String> columnsExcludedFromTypeVerification =
+
MultiStageQueryContext.getColumnsExcludedFromTypeVerification(plannerContext.queryContext());
+ final ArrayIngestMode arrayIngestMode =
MultiStageQueryContext.getArrayIngestMode(plannerContext.queryContext());
+
+ for (Pair<Integer, String> fieldMapping : fieldMappings) {
+ final int columnIndex = fieldMapping.left;
+ final String columnName = fieldMapping.right;
+ final RelDataTypeField oldSqlTypeField =
+
targetTable.getRowType(DruidTypeSystem.TYPE_FACTORY).getField(columnName, true,
false);
+
+ if (!columnsExcludedFromTypeVerification.contains(columnName) &&
oldSqlTypeField != null) {
+ final ColumnType oldDruidType =
Calcites.getColumnTypeForRelDataType(oldSqlTypeField.getType());
+ final RelDataType newSqlType =
rootRel.getRowType().getFieldList().get(columnIndex).getType();
+ final ColumnType newDruidType =
+
DimensionSchemaUtils.getDimensionType(Calcites.getColumnTypeForRelDataType(newSqlType),
arrayIngestMode);
+
+ if (newDruidType.isArray() && oldDruidType.is(ValueType.STRING)
Review Comment:
This seems like we are building a DDL layer, but for specific types, and for
MSQ ingestion, which seems kinda off, given that Druid allows a flexible
schema. However, I also think that most of the cases triggering this would be
accidental, rather than deliberate. Do we plan on getting rid of these
validations, once arrayIngestMode = array is the only acceptable argument to
MSQ ingestion?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
