xiangfu0 commented on code in PR #19284:
URL: https://github.com/apache/pinot/pull/19284#discussion_r3826615269
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/forward/ForwardIndexType.java:
##########
@@ -104,13 +104,19 @@ public ForwardIndexConfig getDefaultConfig() {
@Override
public void validate(FieldIndexConfigs indexConfigs, FieldSpec fieldSpec,
TableConfig tableConfig) {
ForwardIndexConfig forwardIndexConfig =
indexConfigs.getConfig(StandardIndexes.forward());
+ rejectUnsupportedCodecSpec(forwardIndexConfig, fieldSpec.getName());
if (forwardIndexConfig.isEnabled()) {
validateForwardIndexEnabled(forwardIndexConfig, indexConfigs, fieldSpec);
} else {
validateForwardIndexDisabled(indexConfigs, fieldSpec, tableConfig);
}
}
+ static void rejectUnsupportedCodecSpec(ForwardIndexConfig config, String
column) {
+ Preconditions.checkState(!config.hasCodecSpec(),
Review Comment:
Addressed in c7d3f5ed80. Removed rejectUnsupportedCodecSpec and all
temporary creator, reader, mutable-index, reload, and OPEN_STRUCT rejection
guards and tests. The PR description now states explicitly that this unreleased
property is accepted and canonicalized here but remains inactive until #19308.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/codec/CodecSpecParser.java:
##########
@@ -0,0 +1,252 @@
+/**
+ * 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.
+ */
+package org.apache.pinot.segment.spi.codec;
+
+import java.util.ArrayList;
+import java.util.List;
+
+
+/// Stateless and thread-safe structural parser for the codec DSL.
+///
+/// The grammar is:
+/// ```
+/// spec ::= invocation ("," invocation)*
+/// invocation ::= NAME | NAME "(" args ")"
+/// args ::= ε | arg ("," arg)*
+/// arg ::= "0" | [1-9][0-9]*
+/// NAME ::= [A-Za-z_][A-Za-z0-9_]*
+/// ```
+///
+/// Examples are `DELTA`, `ZSTD(3)`, and `DELTA,ZSTD(3)`. The removed
+/// `CODEC(...)` wrapper is deliberately not accepted. This parser validates
only structure;
+/// codec names, arguments, and type compatibility are validated by the codec
runtime.
+///
+/// The grammar is function-call shaped, so reusing Pinot's SQL expression
parsing is a natural
+/// question. `CalciteSqlParser` is not reachable from here: it lives in
`pinot-common`, which
+/// already depends on this module, so calling into it would close a module
cycle. Calcite's own
+/// `SqlParser` is on the classpath — this module uses Calcite for type
inference — but is
+/// deliberately not used either:
+///
+/// - The canonical form produced here is frozen into segment headers and
compared by string
+/// equality to detect rewrites. Delegating canonicalization would tie
on-disk segment
+/// compatibility to Calcite's identifier-casing, quoting, and
literal-formatting rules across
+/// Calcite upgrades.
+/// - A SQL parser accepts far more than this grammar allows — arithmetic,
nested calls, string
+/// literals, aliases, qualified names. The node-tree rejection logic
needed to narrow it back
+/// down would exceed this parser in size, and would fail open as new node
types appear in
+/// later Calcite versions, in a config path that otherwise fails closed.
+/// - Parsing runs on every table-config deserialization, from the
`ForwardIndexConfig`
+/// constructor, on controller, server, and minion.
+///
+/// The cost of that choice is a narrow argument grammar: unsigned integers
only, so neither
+/// negative arguments (such as ZSTD fast-mode levels) nor keyword arguments
are expressible.
+/// Widening this grammar later stays a contained change; removing a Calcite
dependency from
+/// segment headers would not be.
+public final class CodecSpecParser {
+ public static final int MAX_SPEC_LENGTH = 4096;
+ public static final int MAX_PIPELINE_STAGES = 32;
+ public static final int MAX_IDENTIFIER_LENGTH = 128;
+ public static final int MAX_ARGS_PER_INVOCATION = 16;
+ public static final int MAX_ARGUMENT_LENGTH = 32;
+
+ /// Reserved codec name from the removed `CODEC(...)` wrapper syntax. Public
so every layer that
+ /// enforces the reservation (this parser, [CodecInvocation], and the codec
registry) shares one
+ /// definition instead of re-spelling the literal.
+ public static final String REMOVED_WRAPPER_NAME = "CODEC";
+
+ private CodecSpecParser() {
Review Comment:
Addressed in c7d3f5ed80. The private CodecSpecParser constructor is now
immediately after the class declaration.
--
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]
