github-actions[bot] commented on code in PR #69672:
URL: https://github.com/apache/doris/pull/69672#discussion_r4398589131
##########
fe/fe-connector/fe-connector-es/src/main/java/org/apache/doris/connector/es/EsMappingUtils.java:
##########
@@ -150,12 +176,140 @@ public static EsFieldContext
resolveFieldContext(List<String> columnNames,
String fieldType = fieldObject.get("type").asText();
fieldContext.getColumn2typeMap().put(colName, fieldType);
resolveDateFields(fieldContext, fieldObject, colName, fieldType);
- resolveKeywordFields(fieldContext, fieldObject, colName,
fieldType);
resolveDocValuesFields(fieldContext, fieldObject, colName,
fieldType);
}
+ resolveSourceValueSafety(fieldContext, indexMapping, mappingType,
properties);
return fieldContext;
}
+ private static void resolveSourceValueSafety(EsFieldContext context,
String indexMapping, String mappingType,
+ ObjectNode schemaProperties) {
+ try {
+ // Mapping-induced source/index differences on any alias backing
index must prevent
+ // filtering or replacing source values through that path.
+ List<ObjectNode> roots = new ArrayList<>();
+ Set<String> columns = new
HashSet<>(context.getColumn2typeMap().keySet());
+ for (JsonNode index : MAPPER.readTree(indexMapping)) {
+ ObjectNode root = getRootSchema((ObjectNode)
index.get("mappings"), mappingType,
+ new ArrayList<>(), true);
+ roots.add(root);
+ }
+ // Use the same populated schema as inference, including
unprojected filter columns.
+ schemaProperties.fieldNames().forEachRemaining(columns::add);
+ // Filters can reference columns absent from the projection.
Resolve their query paths
+ // from the exposed schema too; the projection still controls
docvalue read eligibility.
+ for (String column : columns) {
+ ObjectNode field = (ObjectNode) schemaProperties.get(column);
+ resolveKeywordFields(context, field, column,
field.path("type").asText());
+ }
+ Set<String> unsafeDocValues = new HashSet<>();
+ for (ObjectNode root : roots) {
+ JsonNode properties = root.path("properties");
+ Set<String> copyTargets = new HashSet<>();
+ collectCopyTargets(properties, copyTargets);
+ for (String column : columns) {
+ JsonNode field = properties.path(column);
+ JsonNode queryField = fieldAtPath(field, column,
context.getFetchFieldsContext().get(column));
+ JsonNode readField = fieldAtPath(field, column,
context.getDocValueFieldsContext().get(column));
+ boolean sourceChanged =
filtersSourceColumn(root.path("_source"), column)
Review Comment:
[P2] Account for disabled root indexing with populated properties
A root `enabled:false` can retain explicit `properties` and `_source`, while
ES [skips parsing all user
fields](https://github.com/elastic/elasticsearch/blob/v8.11.3/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java#L120-L138).
Map `a` as a keyword with a configured lowercase normalizer and `b` as `long`
under that root, then insert `{"a":"ok","b":1}`. The new normalizer check
removes `a`'s docvalue path and forces source reads, but this safety analysis
never checks the root's `enabled` flag. `SELECT a,b FROM idx WHERE b IS NULL`
therefore pushes `must_not exists(b)` and prunes its residual: the unindexed
document matches, yet both readers return `(ok,1)`, violating `IS NULL`. The
merge base read `(NULL,NULL)` through docvalues for that predicate. The current
tests only cover disabled roots without properties, where missing-field checks
hide this case. Unlike the disabled-`_source` thread, source is present here.
Account for root-disabled indexing on every ba
cking and retain the affected source predicates locally.
##########
fe/fe-connector/fe-connector-es/src/main/java/org/apache/doris/connector/es/EsScanPlanProvider.java:
##########
@@ -206,15 +211,13 @@ private ScanNodePropertiesResult buildScanNodeProperties(
// accepts any casing. Passing the raw string through let "TRUE" mean
https to FE and http to BE.
nodeProps.put(PROP_HTTP_SSL_ENABLED,
String.valueOf(props.isHttpSslEnabled()));
- // Query DSL (with not-pushed conjunct tracking)
- EsQueryDslResult dslResult = buildQueryDsl(filter, state);
- nodeProps.put(PROP_QUERY_DSL, dslResult.getQueryDsl());
+ // The read mode is scan-wide: losing a's docvalue path also changes a
keyword sibling b
+ // from scalar docvalues to source JSON (e.g. ["cat"]). Decide it
before pruning b's predicate.
+ int docValuesMode = useDocValueScan(columns, state);
Review Comment:
[P2] Preserve integer decoding when switching siblings to source
With docvalues enabled, map `a` as a keyword with a configured lowercase
normalizer and `b` as ordinary `long`, then insert `{"a":"ok","b":"1.2e3"}`.
ES's [integer
parser](https://github.com/elastic/elasticsearch/blob/v8.11.3/libs/x-content/src/main/java/org/elasticsearch/xcontent/support/AbstractXContentParser.java#L147-L182)
accepts this as 1200. Removing `a`'s path now switches both columns to source,
where `ScrollParser::insert_int_value` strips everything after the first
decimal point and decodes `b` as 1. Because ordinary `long` enters neither
unsafe set, `WHERE b=1200` remains pushed and its residual is pruned, returning
`(ok,1)`; the merge base and numeric-only docvalue control return 1200. This is
integer coercion, distinct from the existing scaled/half-float rounding
threads. The same integer decoder also rejects source `b:[5]` without array
metadata, although its singleton docvalue previously read successfully. Make
the newly selected integer source path compatible with
these supported values, or retain a compatible read path; residuals alone
cannot repair decoding. Cover both readers and docvalue controls.
##########
fe/fe-connector/fe-connector-es/src/main/java/org/apache/doris/connector/es/EsQueryDslBuilder.java:
##########
@@ -406,17 +455,11 @@ private static String
buildComparisonDsl(ConnectorComparison.Operator op,
// col <=> non-null is equivalent to EQ (term queries exclude
nulls)
return termQuery(column, value);
case NE:
- // col != '' means col.length() > 0, NULL should not appear in
results
- if (value instanceof String && ((String) value).isEmpty()) {
- ObjectNode root = MAPPER.createObjectNode();
- ObjectNode boolNode = root.putObject("bool");
- boolNode.set("must_not", parseJsonNode(termQuery(column,
value)));
- boolNode.set("must", parseJsonNode(existsQuery(column)));
- return root.toString();
- }
+ // SQL <> is UNKNOWN for missing values, while ES must_not
alone matches them.
ObjectNode neRoot = MAPPER.createObjectNode();
ObjectNode neBool = neRoot.putObject("bool");
neBool.set("must_not", parseJsonNode(termQuery(column,
value)));
+ neBool.set("must", parseJsonNode(existsQuery(column)));
Review Comment:
[P2] Preserve NULL semantics when negating the new inequality DSL
This regresses when `disable_nereids_expression_rules='SIMPLIFY_NOT_EXPR'`.
With docvalues enabled, `a:long`, `b:keyword`, and `{"a":null,"b":"miss"}`, run
`SELECT a FROM idx WHERE NOT(a <> 1) OR esquery(b, '{"term":{"b":"hit"}}')`.
Disabling that rule preserves the reduced scan filter `OR(NOT(NE(a,1)),
esquery(...))`: `ExpressionTranslator.visitNot` translates the inner negation
to NE and retains the outer NOT. The new `exists(a)` makes the missing row fail
the inner NE; `notToDsl` then wraps it in `must_not`, so the outer NOT succeeds
and the row is returned, although SQL evaluates `UNKNOWN OR false` and excludes
it. The merge-base DSL excluded this row. Default NOT normalization cancels the
double negation, so this finding specifically requires the nondefault session
setting. Normalize the retained negated comparison in this layer, or preserve
its null rejection without depending on that optional rewrite, and add a
regression restoring the session setting afterward.
##########
fe/fe-connector/fe-connector-es/src/main/java/org/apache/doris/connector/es/EsMappingUtils.java:
##########
@@ -66,25 +80,37 @@ public static ObjectNode getMapping(String indexMapping) {
*/
public static ObjectNode getRootSchema(ObjectNode mappings, String
mappingType,
List<String> arrayFields) {
+ return getRootSchema(mappings, mappingType, arrayFields, false);
+ }
+
+ private static ObjectNode getRootSchema(ObjectNode mappings, String
mappingType,
+ List<String> arrayFields, boolean allowEmpty) {
if (mappingType == null) {
- checkNonPropertiesFields(mappings, arrayFields);
- String firstType = mappings.fieldNames().next();
- if (StreamSupport.stream(Spliterators.spliteratorUnknownSize(
- mappings.fieldNames(), Spliterator.ORDERED), false)
- .anyMatch(s -> s.contains("properties"))) {
+ checkNonPropertiesFields(mappings, arrayFields, allowEmpty);
+ // Alias safety analysis must accept backing indices that have not
acquired a mapping yet.
+ if (mappings.isEmpty()) {
return mappings;
- } else {
- ObjectNode firstData = (ObjectNode) mappings.get(firstType);
- checkNonPropertiesFields(firstData, arrayFields);
- return firstData;
}
+ String firstType = mappings.fieldNames().next();
+ // A typeless mapping may contain only flags or root objects such
as _source.
+ // Type wrappers are objects; legacy type names may also be names
of scalar settings.
+ Iterator<String> keys = mappings.fieldNames();
+ while (keys.hasNext()) {
+ String key = keys.next();
+ if (!mappings.get(key).isObject() ||
ROOT_MAPPING_OBJECT_PARAMETERS.contains(key)) {
Review Comment:
[P2] Preserve legacy mapping types named runtime
`runtime` is a valid ES6 mapping type ([type-name
validation](https://github.com/elastic/elasticsearch/blob/v6.8.23/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java#L372-L393)).
For `{"idx":{"mappings":{"runtime":{"properties":{"a":{"type":"long"}}}}}}`,
this name check returns the outer wrapper instead of its populated type.
`EsTypeMapping.parseMapping` always calls this parser with a null mapping type,
so `buildColumns` finds no top-level `properties` and throws `mapping not
found`; setting `mapping_type=runtime` cannot repair schema inference. The
merge-base parser unwrapped this response and exposed `a` as BIGINT. This is
distinct from the empty/configuration-only backing thread: a previously
supported populated legacy schema is now rejected. Distinguish the wrapper's
structure from a typeless root-parameter name, and extend the legacy-type tests
with `runtime`.
--
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]
