Skip to content

Address review feedback from the deprecation-removal PRs #19148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
xiangfu0 merged 3 commits into from
Aug 3, 2026
Merged

Address review feedback from the deprecation-removal PRs #19148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0866c37
Address review feedback from the deprecation-removal PRs
xiangfu0 Aug 2, 2026
fa287ec
Address review feedback: validate both init fields, use a stable 500 …
xiangfu0 Aug 2, 2026
5f3ca4c
Collapse FieldConfig call sites onto one line now that the casts are …
xiangfu0 Aug 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion ...ntroller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableInstances.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,11 @@ public Map<String, List<InstanceInfo>> getLiveBrokers(@Context HttpHeaders heade
try {
return _pinotHelixResourceManager.getTableToLiveBrokersMapping(headers.getHeaderString(DATABASE), tables);
} catch (Exception e) {
throw new ControllerApplicationException(LOGGER, e.getMessage(), Response.Status.NOT_FOUND);
// Unknown tables are filtered out rather than reported, so anything thrown here (e.g. a missing broker
// ExternalView) is a server-side failure, not a lookup miss. Use a stable message since the cause may
// carry none, and let the attached cause supply the detail in the logs.
throw new ControllerApplicationException(LOGGER, "Failed to get table to live brokers mapping",
Response.Status.INTERNAL_SERVER_ERROR, e);
}
}

Expand Down
10 changes: 3 additions & 7 deletions ...est/java/org/apache/pinot/core/query/aggregation/function/AvgAggregationFunctionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
*/
package org.apache.pinot.core.query.aggregation.function;

import java.util.List;
import org.apache.pinot.queries.FluentQueryTest;
import org.apache.pinot.spi.config.table.FieldConfig;
import org.apache.pinot.spi.config.table.TableType;
Expand Down Expand Up @@ -196,8 +195,7 @@ void singleKeyAggregationWithSmallNumGroupsLimitDoesntThrowAIOOBE(FieldConfig.En
.build(),
new TableConfigBuilder(TableType.OFFLINE)
.setTableName("testTable")
.addFieldConfig(
new FieldConfig("key", encoding, (List<FieldConfig.IndexType>) null, PASS_THROUGH, null))
.addFieldConfig(new FieldConfig("key", encoding, null, PASS_THROUGH, null))
.build())
.onFirstInstance(new Object[]{7, 1}, new Object[]{6, 2}, new Object[]{5, 3}, new Object[]{4, 4})
.andOnSecondInstance(new Object[]{7, 1}, new Object[]{6, 2}, new Object[]{5, 3}, new Object[]{4, 4})
Expand Down Expand Up @@ -228,10 +226,8 @@ void multiKeyAggregationWithSmallNumGroupsLimitDoesntThrowAIOOBE(FieldConfig.Enc
.build(),
new TableConfigBuilder(TableType.OFFLINE)
.setTableName("testTable")
.addFieldConfig(
new FieldConfig("key1", encoding, (List<FieldConfig.IndexType>) null, PASS_THROUGH, null))
.addFieldConfig(
new FieldConfig("key2", encoding, (List<FieldConfig.IndexType>) null, PASS_THROUGH, null))
.addFieldConfig(new FieldConfig("key1", encoding, null, PASS_THROUGH, null))
.addFieldConfig(new FieldConfig("key2", encoding, null, PASS_THROUGH, null))
.build())
.onFirstInstance(new Object[]{7, 1}, new Object[]{6, 2}, new Object[]{5, 3}, new Object[]{4, 4})
.andOnSecondInstance(new Object[]{7, 1}, new Object[]{6, 2}, new Object[]{5, 3}, new Object[]{4, 4})
Expand Down
18 changes: 11 additions & 7 deletions ...pache/pinot/plugin/minion/tasks/materializedview/MaterializedViewTaskExecutorFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/
package org.apache.pinot.plugin.minion.tasks.materializedview;

import com.google.common.base.Preconditions;
import org.apache.pinot.common.config.GrpcConfig;
import org.apache.pinot.materializedview.executor.GrpcMaterializedViewQueryExecutor;
import org.apache.pinot.materializedview.executor.MaterializedViewQueryExecutor;
Expand Down Expand Up @@ -59,18 +60,21 @@ public String getTaskType() {

@Override
public PinotTaskExecutor create() {
// Validated on every call rather than only on the first: without a MinionConf we would silently build a
// plaintext gRPC client with default limits instead of the configured one, and without a metadata manager
// we would hand a null straight to the executor.
Preconditions.checkState(_zkMetadataManager != null,
"MinionTaskZkMetadataManager is not set; init(zkMetadataManager, minionConf) must be called before create()");
Preconditions.checkState(_minionConf != null,
"MinionConf is not set; init(zkMetadataManager, minionConf) must be called before create()");
if (_queryExecutor == null) {
synchronized (this) {
if (_queryExecutor == null) {
// Build the gRPC client config from the minion's own configuration, scoped to the
// MaterializedViewTask.MINION_BROKER_GRPC_CONFIG_PREFIX prefix. This is how operators
// enable TLS, raise the max inbound message size for large MV result sets, and tune
// keepalive. Falling back to an empty configuration (no TLS, defaults) when no
// MinionConf was provided — fine for local tests but production deployments should
// initialize the factory with a MinionConf.
PinotConfiguration grpcClientConfig = _minionConf != null
? _minionConf.subset(MaterializedViewTask.MINION_BROKER_GRPC_CONFIG_PREFIX)
: new PinotConfiguration();
// enable TLS, raise the max inbound message size for large MV result sets, and tune keepalive.
PinotConfiguration grpcClientConfig =
_minionConf.subset(MaterializedViewTask.MINION_BROKER_GRPC_CONFIG_PREFIX);
_queryExecutor = new GrpcMaterializedViewQueryExecutor(
MinionContext.getInstance().getHelixManager(),
new GrpcConfig(grpcClientConfig));
Expand Down
14 changes: 6 additions & 8 deletions ...al/src/test/java/org/apache/pinot/segment/local/utils/IndexCombinationValidationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ public void testForwardIndexDisabledWithDictAndInvertedPasses() {

@Test
public void testRawWithLz4CodecPasses() {
FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW, (List<IndexType>) null, CompressionCodec.LZ4, null);
FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW, null, CompressionCodec.LZ4, null);
TableConfig tc = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
.setNoDictionaryColumns(List.of(STR_COL))
.setFieldConfigList(List.of(fc))
Expand All @@ -468,7 +468,7 @@ public void testRawWithLz4CodecPasses() {

@Test
public void testRawWithSnappyCodecPasses() {
FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW, (List<IndexType>) null, CompressionCodec.SNAPPY, null);
FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW, null, CompressionCodec.SNAPPY, null);
TableConfig tc = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
.setNoDictionaryColumns(List.of(STR_COL))
.setFieldConfigList(List.of(fc))
Expand All @@ -478,8 +478,7 @@ public void testRawWithSnappyCodecPasses() {

@Test
public void testRawWithZstdCodecPasses() {
FieldConfig fc =
new FieldConfig(STR_COL, EncodingType.RAW, (List<IndexType>) null, CompressionCodec.ZSTANDARD, null);
FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW, null, CompressionCodec.ZSTANDARD, null);
TableConfig tc = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
.setNoDictionaryColumns(List.of(STR_COL))
.setFieldConfigList(List.of(fc))
Expand All @@ -490,7 +489,7 @@ public void testRawWithZstdCodecPasses() {
@Test
public void testRawWithClpCodecStringColumnPasses() {
// CLP codecs are valid for raw STRING columns
FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW, (List<IndexType>) null, CompressionCodec.CLP, null);
FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW, null, CompressionCodec.CLP, null);
TableConfig tc = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
.setNoDictionaryColumns(List.of(STR_COL))
.setFieldConfigList(List.of(fc))
Expand All @@ -501,7 +500,7 @@ public void testRawWithClpCodecStringColumnPasses() {
@Test
public void testRawWithClpCodecNonStringColumnFails() {
// CLP is only valid on STRING stored type
FieldConfig fc = new FieldConfig(INT_COL, EncodingType.RAW, (List<IndexType>) null, CompressionCodec.CLP, null);
FieldConfig fc = new FieldConfig(INT_COL, EncodingType.RAW, null, CompressionCodec.CLP, null);
TableConfig tc = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
.setNoDictionaryColumns(List.of(INT_COL))
.setFieldConfigList(List.of(fc))
Expand All @@ -512,8 +511,7 @@ public void testRawWithClpCodecNonStringColumnFails() {
@Test
public void testDeltaDeltaCodecNonNumericColumnFails() {
// DELTADELTA only valid on INT/LONG columns
FieldConfig fc =
new FieldConfig(STR_COL, EncodingType.RAW, (List<IndexType>) null, CompressionCodec.DELTADELTA, null);
FieldConfig fc = new FieldConfig(STR_COL, EncodingType.RAW, null, CompressionCodec.DELTADELTA, null);
TableConfig tc = new TableConfigBuilder(TableType.OFFLINE).setTableName(TABLE_NAME)
.setNoDictionaryColumns(List.of(STR_COL))
.setFieldConfigList(List.of(fc))
Expand Down
9 changes: 3 additions & 6 deletions pinot-spi/src/test/java/org/apache/pinot/spi/config/table/OpenStructIndexConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ public void testDisabledConfig() {

@Test
public void testNoDictionaryKeys() {
FieldConfig rawKey =
new FieldConfig("raw_payload", FieldConfig.EncodingType.RAW, (List<FieldConfig.IndexType>) null, null, null);
FieldConfig rawKey = new FieldConfig("raw_payload", FieldConfig.EncodingType.RAW, null, null, null);
OpenStructIndexConfig config = new OpenStructIndexConfig(false, null, 1000, null, 0.5, List.of(rawKey));
assertFalse(config.shouldUseDictionaryForKey("raw_payload"));
// Unconfigured key falls back to built-in default (DICTIONARY).
Expand Down Expand Up @@ -125,10 +124,8 @@ public void testDefaultValueFieldConfigRawEncoding()

@Test
public void testShouldUseDictionaryForKeyHardOverride() {
FieldConfig blob =
new FieldConfig("blob", FieldConfig.EncodingType.RAW, (List<FieldConfig.IndexType>) null, null, null);
FieldConfig rawPayload =
new FieldConfig("raw_payload", FieldConfig.EncodingType.RAW, (List<FieldConfig.IndexType>) null, null, null);
FieldConfig blob = new FieldConfig("blob", FieldConfig.EncodingType.RAW, null, null, null);
FieldConfig rawPayload = new FieldConfig("raw_payload", FieldConfig.EncodingType.RAW, null, null, null);
OpenStructIndexConfig config = new OpenStructIndexConfig(false, null, 1000, null, 0.5, List.of(blob, rawPayload));
assertFalse(config.shouldUseDictionaryForKey("blob"));
assertFalse(config.shouldUseDictionaryForKey("raw_payload"));
Expand Down
Loading