xiangfu0 commented on code in PR #19284:
URL: https://github.com/apache/pinot/pull/19284#discussion_r3827224798
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/ForwardIndexHandler.java:
##########
@@ -289,6 +290,14 @@ Map<String, List<Operation>>
computeOperations(SegmentDirectory.Reader segmentRe
return columnOperationsMap;
}
+ private void rejectUnsupportedCodecSpecs() {
Review Comment:
Addressed in c7d3f5ed80. Removed the preprocessing rejection together with
the other temporary unsupported-codecSpec guards and their regression tests,
keeping this as the requested stacked configuration layer.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -262,15 +287,16 @@ public boolean equals(Object o) {
return false;
}
ForwardIndexConfig that = (ForwardIndexConfig) o;
- return _compressionCodec == that._compressionCodec &&
_deriveNumDocsPerChunk == that._deriveNumDocsPerChunk
+ return _compressionCodec == that._compressionCodec &&
Objects.equals(_codecSpec, that._codecSpec)
Review Comment:
Addressed in c7d3f5ed80. ForwardIndexConfig.equals now keeps one comparison
per line, and the final Spotless and Checkstyle passes are clean on both
affected modules.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/ForwardIndexConfig.java:
##########
@@ -207,6 +220,18 @@ public CompressionCodec getCompressionCodec() {
return _compressionCodec;
}
+ /// Returns the structurally normalized codec specification, or `null` for
the legacy compression path.
+ @Nullable
+ public String getCodecSpec() {
+ return _codecSpec;
+ }
+
+ /// Returns whether this config contains a codec specification.
+ @JsonIgnore
+ public boolean hasCodecSpec() {
Review Comment:
Addressed in c7d3f5ed80. Removed hasCodecSpec; callers and tests use the
nullable getCodecSpec contract directly.
##########
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";
Review Comment:
Addressed in c7d3f5ed80 and 29eddbd468. Removed the public removed-wrapper
constant. The reserved-name rule and the rest of the grammar invariants now
live in package-private CodecDslSyntax, while the parser keeps its early,
specific diagnostic.
--
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]
