github-actions[bot] commented on code in PR #67918:
URL: https://github.com/apache/doris/pull/67918#discussion_r4081551206
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/invertedindex/AnalyzerIdentityBuilder.java:
##########
@@ -45,14 +113,61 @@ public static String buildAnalyzerIdentity(
}
if (!Strings.isNullOrEmpty(preferredAnalyzer)) {
+ String builtinIkIdentity =
resolveBuiltinIkAnalyzerIdentity(properties, preferredAnalyzer);
+ if (builtinIkIdentity != null) {
+ return appendOuterCharFilterIdentity(
+ builtinIkIdentity, properties,
builtinIkFoldContext(builtinIkIdentity));
+ }
// For custom analyzer/normalizer, resolve to underlying config to
build identity
- return resolveAnalyzerIdentity(preferredAnalyzer,
defaultAnalyzerKey, log);
+ return appendOuterCharFilterIdentity(
+ resolveAnalyzerIdentity(preferredAnalyzer,
defaultAnalyzerKey, log), properties,
+ customAnalyzerFoldContext(preferredAnalyzer));
}
if (Strings.isNullOrEmpty(parser) ||
parserNone.equalsIgnoreCase(parser)) {
return defaultAnalyzerKey;
}
- return parser;
+ String legacyIkIdentity = resolveLegacyIkIdentity(properties, parser);
+ if (legacyIkIdentity != null) {
+ return appendOuterCharFilterIdentity(
+ legacyIkIdentity, properties,
builtinIkFoldContext(legacyIkIdentity));
+ }
+ return appendOuterCharFilterIdentity(parser, properties, null);
Review Comment:
[P2] Canonicalize the standard/unicode analyzer alias. This branch preserves
the parser spelling, so an otherwise-identical standard/unicode pair gets
different identities and selectors and passes both CREATE and ALTER duplicate
fences. BE handles PARSER_STANDARD and PARSER_UNICODE in the same
create_builtin_analyzer branch and constructs StandardAnalyzer for both, while
this feature otherwise rejects analyzers with the same effective configuration.
Please map these aliases to one identity and cover both DDL paths.
##########
regression-test/suites/inverted_index_p0/analyzer/test_analyzer_malformed_utf8_write.groovy:
##########
@@ -0,0 +1,110 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// A string column may hold bytes that are not valid UTF-8. Indexing them must
keep working:
+// the malformed bytes are skipped and the valid text around them stays
searchable.
+suite("test_analyzer_malformed_utf8_write", "p0") {
+ def ngramTable = "test_malformed_utf8_ngram"
+ def icuTable = "test_malformed_utf8_icu"
+
+ sql "DROP TABLE IF EXISTS ${ngramTable}"
+ sql "DROP TABLE IF EXISTS ${icuTable}"
+
+ sql """
+ CREATE INVERTED INDEX TOKENIZER IF NOT EXISTS
malformed_utf8_ngram_tokenizer
+ PROPERTIES
+ (
+ "type" = "ngram",
+ "min_gram" = "2",
+ "max_gram" = "2"
+ );
+ """
+
+ sql """
+ CREATE INVERTED INDEX ANALYZER IF NOT EXISTS
malformed_utf8_ngram_analyzer
+ PROPERTIES
+ (
+ "tokenizer" = "malformed_utf8_ngram_tokenizer"
+ );
+ """
+
+ sql """
+ CREATE INVERTED INDEX ANALYZER IF NOT EXISTS
malformed_utf8_icu_analyzer
+ PROPERTIES
+ (
+ "tokenizer" = "icu"
+ );
+ """
+
+ for (String analyzer : ["malformed_utf8_ngram_analyzer",
"malformed_utf8_icu_analyzer"]) {
+ Exception lastException = null
+ boolean ready = false
+ for (int attempt = 0; attempt < 30; attempt++) {
+ try {
+ sql """SELECT TOKENIZE('probe', '"analyzer"="${analyzer}"')"""
+ ready = true
+ break
+ } catch (Exception e) {
+ lastException = e
+ sleep(1000)
+ }
+ }
+ assertTrue(ready, "Analyzer ${analyzer} was not ready:
${lastException?.message}")
+ }
+
+ sql """
+ CREATE TABLE ${ngramTable} (
+ `id` int NOT NULL,
+ `ch` text NULL,
+ INDEX idx_ch (`ch`) USING INVERTED PROPERTIES("analyzer" =
"malformed_utf8_ngram_analyzer")
+ ) ENGINE=OLAP
+ DUPLICATE KEY(`id`)
+ DISTRIBUTED BY HASH(`id`) BUCKETS 1
+ PROPERTIES ("replication_allocation" = "tag.location.default: 1");
+ """
+
+ sql """
+ CREATE TABLE ${icuTable} (
+ `id` int NOT NULL,
+ `ch` text NULL,
+ INDEX idx_ch (`ch`) USING INVERTED PROPERTIES("analyzer" =
"malformed_utf8_icu_analyzer")
+ ) ENGINE=OLAP
+ DUPLICATE KEY(`id`)
+ DISTRIBUTED BY HASH(`id`) BUCKETS 1
+ PROPERTIES ("replication_allocation" = "tag.location.default: 1");
+ """
+
+ // An overlong encoding and a byte that can never start a sequence.
+ sql """ INSERT INTO ${ngramTable} VALUES (1, CONCAT('abcd', UNHEX('C0AF'),
'efgh')) """
+ sql """ INSERT INTO ${ngramTable} VALUES (2, CONCAT('wxyz', UNHEX('FF')))
"""
+ sql """ INSERT INTO ${ngramTable} VALUES (3, 'plain') """
+ sql """ INSERT INTO ${icuTable} VALUES (1, CONCAT('alpha ', UNHEX('FF'), '
beta')) """
+ sql """ INSERT INTO ${icuTable} VALUES (2, 'gamma delta') """
+
+ sql "sync"
+
+ // Every row was written, so the malformed bytes did not fail the index
write.
+ assertEquals(3, sql("SELECT COUNT(*) FROM ${ngramTable}")[0][0])
Review Comment:
[P2] Record these deterministic results as golden queries. The stable row
and MATCH counts on lines 101-109 are checked only with assertEquals, and this
change adds no corresponding regression-test data .out file. The repository
test contract requires determined results to use qt_sql/order_qt_sql (or
similar) so the runner generates and reviews the expected output. Please
convert these checks and add the generated .out.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/invertedindex/AnalyzerIdentityBuilder.java:
##########
@@ -45,14 +113,61 @@ public static String buildAnalyzerIdentity(
}
if (!Strings.isNullOrEmpty(preferredAnalyzer)) {
+ String builtinIkIdentity =
resolveBuiltinIkAnalyzerIdentity(properties, preferredAnalyzer);
+ if (builtinIkIdentity != null) {
+ return appendOuterCharFilterIdentity(
+ builtinIkIdentity, properties,
builtinIkFoldContext(builtinIkIdentity));
+ }
// For custom analyzer/normalizer, resolve to underlying config to
build identity
- return resolveAnalyzerIdentity(preferredAnalyzer,
defaultAnalyzerKey, log);
+ return appendOuterCharFilterIdentity(
+ resolveAnalyzerIdentity(preferredAnalyzer,
defaultAnalyzerKey, log), properties,
Review Comment:
[P2] Canonicalize built-ins against their effective component graphs. The
built-in fast path in resolveAnalyzerIdentity returns bare names, while named
custom analyzers with tokenizer=basic/icu plus token_filter=lowercase get
component identities; their distinct selectors then let both CREATE and ALTER
accept each pair. BE's BasicAnalyzer and ICUAnalyzer are exactly those
tokenizer-plus-LowerCaseFilter pipelines under default settings (including
ICU's same dictionary path). Please give these built-ins the matching effective
identities and cover both DDL paths.
--
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]
