You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2026/03/22 02:54:14 UTC
[PR] Add useRawBytes config to Murmur and Murmur3 partition functions [pinot]
xiangfu0 opened a new pull request, #17932:
URL: https://github.com/apache/pinot/pull/17932
## Summary
- Add `useRawBytes` option to `functionConfig` for `MurmurPartitionFunction` and `Murmur3PartitionFunction` (defaults to `false`)
- When `useRawBytes=true`, the hex-encoded partition value is decoded back to raw bytes via `BytesUtils.toBytes()` before hashing, instead of treating it as UTF-8 text via `value.getBytes(UTF_8)`
- This enables correct partition assignment for BYTES columns where the value is hex-encoded but should be hashed on the original raw bytes
## Test plan
- [x] Added `testMurmurPartitionFunctionUseRawBytes` — verifies hex-decoded bytes produce expected murmur2 hash partition, and that default behavior is unchanged
- [x] Added `testMurmur3PartitionFunctionUseRawBytes` — verifies hex-decoded bytes for x86_32, x64_32 variants, and non-zero seed, and that default behavior is unchanged
- [x] All existing `PartitionFunctionTest` tests pass
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add useRawBytes config to Murmur and Murmur3 partition functions [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17932:
URL: https://github.com/apache/pinot/pull/17932#discussion_r2970717114
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/Murmur3PartitionFunction.java:
##########
@@ -45,9 +49,11 @@ public class Murmur3PartitionFunction implements PartitionFunction {
public Murmur3PartitionFunction(int numPartitions, Map<String, String> functionConfig) {
Preconditions.checkArgument(numPartitions > 0, "Number of partitions must be > 0");
_numPartitions = numPartitions;
+ _functionConfig = functionConfig;
Review Comment:
This class now stores `functionConfig` and may receive `null` (the factory passes a `@Nullable` config). However `_functionConfig` is declared non-null and the constructor parameter isn’t annotated, which can cause nullness/static-analysis warnings. Please mark the parameter/field (and `getFunctionConfig()` override) as `@Nullable` (or normalize to an empty map).
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/Murmur3PartitionFunction.java:
##########
@@ -62,13 +68,21 @@ public Murmur3PartitionFunction(int numPartitions, Map<String, String> functionC
"Murmur3 variant must be either x86_32 or x64_32");
}
}
+ useRawBytes = Boolean.parseBoolean(functionConfig.get(USE_RAW_BYTES_KEY));
}
_seed = seed;
_useX64 = useX64;
+ _useRawBytes = useRawBytes;
Review Comment:
Like `MurmurPartitionFunction`, this stores the caller-provided `functionConfig` map by reference. Because `_seed/_useX64/_useRawBytes` are parsed once in the constructor, later mutations to the original map can make `getFunctionConfig()` (and JSON serialization) inconsistent with the instance’s actual behavior. Consider defensively copying (and possibly wrapping as unmodifiable) at construction time.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/MurmurPartitionFunction.java:
##########
@@ -55,6 +73,11 @@ public int getNumPartitions() {
return _numPartitions;
}
+ @Override
+ public Map<String, String> getFunctionConfig() {
+ return _functionConfig;
+ }
Review Comment:
`getFunctionConfig()` can return `null` (e.g., when constructed via `MurmurPartitionFunction(int)`), but the override isn’t annotated as nullable and the backing field isn’t marked nullable. Please annotate the override (and ideally `_functionConfig`) with `@Nullable` to match `PartitionFunction#getFunctionConfig()` and avoid nullness/IDE warnings.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/MurmurPartitionFunction.java:
##########
@@ -29,20 +32,35 @@
*/
public class MurmurPartitionFunction implements PartitionFunction {
private static final String NAME = "Murmur";
+ private static final String USE_RAW_BYTES_KEY = "useRawBytes";
private final int _numPartitions;
+ private final Map<String, String> _functionConfig;
+ private final boolean _useRawBytes;
/**
- * Constructor for the class.
+ * Constructor for backward compatibility.
* @param numPartitions Number of partitions.
*/
public MurmurPartitionFunction(int numPartitions) {
+ this(numPartitions, null);
+ }
+
+ /**
+ * Constructor for the class.
+ * @param numPartitions Number of partitions.
+ * @param functionConfig to extract configurations for the partition function.
+ */
+ public MurmurPartitionFunction(int numPartitions, @Nullable Map<String, String> functionConfig) {
Preconditions.checkArgument(numPartitions > 0, "Number of partitions must be > 0");
_numPartitions = numPartitions;
+ _functionConfig = functionConfig;
+ _useRawBytes = functionConfig != null && Boolean.parseBoolean(functionConfig.get(USE_RAW_BYTES_KEY));
}
Review Comment:
The constructor stores the caller-provided `functionConfig` map by reference. If that map is later mutated, `getFunctionConfig()`/JSON output can diverge from the already-parsed `_useRawBytes` behavior (and could even trigger concurrent modification issues during serialization). Consider taking a defensive copy (optionally wrapping it as unmodifiable) so the exposed config is a stable snapshot of what this instance was built with.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add useRawBytes config to Murmur and Murmur3 partition functions [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17932:
URL: https://github.com/apache/pinot/pull/17932#discussion_r2970809038
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/Murmur3PartitionFunction.java:
##########
@@ -33,21 +36,27 @@ public class Murmur3PartitionFunction implements PartitionFunction {
private static final String NAME = "Murmur3";
private static final String SEED_KEY = "seed";
private static final String VARIANT_KEY = "variant";
+ private static final String USE_RAW_BYTES_KEY = "useRawBytes";
private final int _numPartitions;
+ @Nullable
+ private final Map<String, String> _functionConfig;
private final int _seed;
private final boolean _useX64;
+ private final boolean _useRawBytes;
/**
* Constructor for the class.
* @param numPartitions Number of partitions.
* @param functionConfig to extract configurations for the partition function.
*/
- public Murmur3PartitionFunction(int numPartitions, Map<String, String> functionConfig) {
+ public Murmur3PartitionFunction(int numPartitions, @Nullable Map<String, String> functionConfig) {
Preconditions.checkArgument(numPartitions > 0, "Number of partitions must be > 0");
_numPartitions = numPartitions;
+ _functionConfig = functionConfig != null ? Collections.unmodifiableMap(functionConfig) : null;
int seed = 0;
boolean useX64 = false;
+ boolean useRawBytes = false;
if (functionConfig != null) {
Review Comment:
`Collections.unmodifiableMap(functionConfig)` does not prevent external mutation of the underlying `functionConfig` map (it’s a live view). If the original map is modified after constructing the partition function, `getFunctionConfig()` will change while `_seed/_useX64/_useRawBytes` remain based on the old values, causing inconsistent serialization/debugging and potential concurrency issues. Consider copying the map first (e.g., `Map.copyOf(functionConfig)` or wrap a new `HashMap`).
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/PartitionFunctionFactory.java:
##########
@@ -77,7 +77,7 @@ public static PartitionFunction getPartitionFunction(String functionName, int nu
case Murmur:
case Murmur2:
- return new MurmurPartitionFunction(numPartitions);
+ return new MurmurPartitionFunction(numPartitions, functionConfig);
case Murmur3:
return new Murmur3PartitionFunction(numPartitions, functionConfig);
Review Comment:
The new `functionConfig` forwarding for Murmur/Murmur2 in `PartitionFunctionFactory` (needed for `useRawBytes`) isn’t covered by the added tests because they instantiate `MurmurPartitionFunction`/`Murmur3PartitionFunction` directly. Consider adding an assertion/test that constructs these partition functions via `PartitionFunctionFactory.getPartitionFunction(...)` with `useRawBytes=true` (and Murmur2 alias) to ensure the factory wiring works end-to-end.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add useRawBytes config to Murmur and Murmur3 partition functions [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17932:
URL: https://github.com/apache/pinot/pull/17932#discussion_r2970708916
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/MurmurPartitionFunction.java:
##########
@@ -29,20 +32,25 @@
*/
public class MurmurPartitionFunction implements PartitionFunction {
private static final String NAME = "Murmur";
+ private static final String USE_RAW_BYTES_KEY = "useRawBytes";
private final int _numPartitions;
+ private final boolean _useRawBytes;
/**
* Constructor for the class.
* @param numPartitions Number of partitions.
+ * @param functionConfig to extract configurations for the partition function.
*/
- public MurmurPartitionFunction(int numPartitions) {
+ public MurmurPartitionFunction(int numPartitions, @Nullable Map<String, String> functionConfig) {
Preconditions.checkArgument(numPartitions > 0, "Number of partitions must be > 0");
_numPartitions = numPartitions;
+ _useRawBytes = functionConfig != null && Boolean.parseBoolean(functionConfig.get(USE_RAW_BYTES_KEY));
}
Review Comment:
`MurmurPartitionFunction` previously exposed a public `MurmurPartitionFunction(int numPartitions)` constructor; removing it is an API-breaking change for any downstream code instantiating this SPI directly. Consider re-introducing the original constructor as an overload delegating to the new `(int, Map<String,String>)` constructor to preserve backward compatibility.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/Murmur3PartitionFunction.java:
##########
@@ -62,13 +66,21 @@ public Murmur3PartitionFunction(int numPartitions, Map<String, String> functionC
"Murmur3 variant must be either x86_32 or x64_32");
}
}
+ useRawBytes = Boolean.parseBoolean(functionConfig.get(USE_RAW_BYTES_KEY));
}
_seed = seed;
_useX64 = useX64;
+ _useRawBytes = useRawBytes;
Review Comment:
`useRawBytes` is parsed from `functionConfig`, but the class doesn’t expose any `functionConfig` via `getFunctionConfig()`. As a result, partition metadata serialization (e.g., to ZK) can lose `useRawBytes` (and `seed`/`variant`), and query routing that reconstructs the function from metadata can compute different partitions. Persist the effective config (preferably as an immutable copy) and override `getFunctionConfig()` to return it.
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/partition/MurmurPartitionFunction.java:
##########
@@ -29,20 +32,25 @@
*/
public class MurmurPartitionFunction implements PartitionFunction {
private static final String NAME = "Murmur";
+ private static final String USE_RAW_BYTES_KEY = "useRawBytes";
private final int _numPartitions;
+ private final boolean _useRawBytes;
/**
* Constructor for the class.
* @param numPartitions Number of partitions.
+ * @param functionConfig to extract configurations for the partition function.
*/
- public MurmurPartitionFunction(int numPartitions) {
+ public MurmurPartitionFunction(int numPartitions, @Nullable Map<String, String> functionConfig) {
Preconditions.checkArgument(numPartitions > 0, "Number of partitions must be > 0");
_numPartitions = numPartitions;
+ _useRawBytes = functionConfig != null && Boolean.parseBoolean(functionConfig.get(USE_RAW_BYTES_KEY));
}
@Override
public int getPartition(String value) {
- return (MurmurHashFunctions.murmurHash2(value.getBytes(UTF_8)) & Integer.MAX_VALUE) % _numPartitions;
+ byte[] bytes = _useRawBytes ? BytesUtils.toBytes(value) : value.getBytes(UTF_8);
+ return (MurmurHashFunctions.murmurHash2(bytes) & Integer.MAX_VALUE) % _numPartitions;
Review Comment:
`useRawBytes` is derived from `functionConfig`, but this partition function doesn’t override `getFunctionConfig()` to expose that config. That can cause partition metadata written via `SegmentZKMetadataUtils` to drop the config, and brokers reconstructing the function from ZK metadata to fall back to default behavior, yielding incorrect partition routing/pruning. Persist the effective config (or at least `useRawBytes=true`) and return it from `getFunctionConfig()`.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add useRawBytes config to Murmur and Murmur3 partition functions [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17932:
URL: https://github.com/apache/pinot/pull/17932#issuecomment-4105488296
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17932?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `95.00000%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.46%. Comparing base ([`550afb7`](https://app.codecov.io/gh/apache/pinot/commit/550afb77119492bd612d07011d7f3051d1a4a782?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5f21f1c`](https://app.codecov.io/gh/apache/pinot/commit/5f21f1c3092602df2306663366802088d8ae7a77?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17932?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...segment/spi/partition/MurmurPartitionFunction.java](https://app.codecov.io/gh/apache/pinot/pull/17932?src=pr&el=tree&filepath=pinot-segment-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Fspi%2Fpartition%2FMurmurPartitionFunction.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL3BhcnRpdGlvbi9NdXJtdXJQYXJ0aXRpb25GdW5jdGlvbi5qYXZh) | 87.50% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17932?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (550afb7) and HEAD (5f21f1c). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (550afb7) | HEAD (5f21f1c) |
>|------|------|------|
>|java-21|5|1|
>|unittests1|2|1|
>|unittests|4|1|
>|temurin|10|1|
>|java-11|5|0|
>|unittests2|2|0|
>|integration|6|0|
>|integration2|2|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #17932 +/- ##
============================================
- Coverage 63.19% 55.46% -7.73%
+ Complexity 1481 729 -752
============================================
Files 3191 2494 -697
Lines 192592 141645 -50947
Branches 29537 22624 -6913
============================================
- Hits 121710 78568 -43142
+ Misses 61356 56409 -4947
+ Partials 9526 6668 -2858
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17932/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/17932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/17932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/17932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/17932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/17932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/17932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.46% <95.00%> (-7.70%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.46% <95.00%> (-7.73%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.46% <95.00%> (-7.73%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.46% <95.00%> (-0.03%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17932/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/17932?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add useRawBytes config to Murmur and Murmur3 partition functions [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #17932:
URL: https://github.com/apache/pinot/pull/17932
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org