You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "cecemei (via GitHub)" <gi...@apache.org> on 2025/09/17 04:18:16 UTC
[PR] Add dictionary size estimate in `DictionaryBuildingSingleValuedRowBasedKeySerdeHelper` (druid)
cecemei opened a new pull request, #18541:
URL: https://github.com/apache/druid/pull/18541
### Description
Add dictionary size estimate in `DictionaryBuildingSingleValuedRowBasedKeySerdeHelper`.
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
#### Release note
<!-- Give your best effort to summarize your changes in a couple of sentences aimed toward Druid users.
If your change doesn't have end user impact, you can skip this section.
For tips about how to write a good release note, see [Release notes](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#release-notes).
-->
<hr>
##### Key changed/added classes in this PR
* `MyFoo`
* `OurBar`
* `TheirBaz`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add size check in DictionaryBuildingSingleValuedRowBasedKeySerdeHelper in putToKeyBuffer (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2396153140
##########
processing/src/main/java/org/apache/druid/segment/nested/NestedDataComplexTypeSerde.java:
##########
@@ -124,7 +123,7 @@ public Object fromByteBuffer(ByteBuffer buffer, int numBytes)
@Override
public byte[] toBytes(@Nullable Object val)
{
- return serializeToBytes(val);
+ return serializeToBytes(StructuredData.wrap(val));
Review Comment:
true, the wrap is implemented in deserializer, removed here
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add dictionary size estimate in `DictionaryBuildingSingleValuedRowBasedKeySerdeHelper` (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2354283937
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java:
##########
@@ -1692,6 +1702,23 @@
}
}
+ @Override
+ int estimatedKeySize(RowBasedKey key)
+ {
+ Object[] obj = key.getKey();
+ int size = 0;
+ for (Object o : obj) {
+ if (o instanceof String) {
+ size += estimateStringKeySize((String) o);
Review Comment:
## Implicit narrowing conversion in compound assignment
Implicit cast of source type long to narrower destination type [int](1).
[Show more details](https://github.com/apache/druid/security/code-scanning/10335)
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java:
##########
@@ -1692,6 +1702,23 @@
}
}
+ @Override
+ int estimatedKeySize(RowBasedKey key)
+ {
+ Object[] obj = key.getKey();
+ int size = 0;
+ for (Object o : obj) {
+ if (o instanceof String) {
+ size += estimateStringKeySize((String) o);
+ } else if (o != null) {
+ size += estimateStringKeySize(String.valueOf(o));
Review Comment:
## Implicit narrowing conversion in compound assignment
Implicit cast of source type long to narrower destination type [int](1).
[Show more details](https://github.com/apache/druid/security/code-scanning/10336)
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add size check in DictionaryBuildingSingleValuedRowBasedKeySerdeHelper in putToKeyBuffer (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2408846105
##########
processing/src/main/java/org/apache/druid/segment/nested/StructuredData.java:
##########
@@ -133,16 +135,28 @@ private Number asNumber()
return (Number) value;
}
+ @SuppressWarnings("ReturnValueIgnored")
+ public int getSizeEstimate()
+ {
+ if (sizeEstimate < 0) {
+ hash.getAsLong(); // trigger hash computation which also sets sizeEstimate
+ }
+ Preconditions.checkState(sizeEstimate >= 0, "sizeEstimate not initialized");
+ return sizeEstimate;
+ }
+
@Override
public int compareTo(StructuredData o)
{
- if (this.equals(o)) {
+ if (this == o) {
Review Comment:
i feel like we should leave this, since `StructuredData` is also used as a wrapper at ingest time for all 'auto' types, primitive types can short-circuit out of this comparison before forcing stuff to be serialized, which seems like it would add a bit of overhead.
I guess you're trying to make all of equals/hashcode/compareTo like consistently weird the same way with the occasional hash collisions, instead of just isolating the weirdness where it falls to the hash in just the compareTo?
Fwiw, I was trying to keep the strangeness of using the hash instead of performing full byte comparison isolated in compareTo (since ideally we would be performing full comparison), is there a reason related to this PR of why these need to change?
##########
processing/src/main/java/org/apache/druid/segment/column/TypeStrategies.java:
##########
@@ -697,4 +704,103 @@ public Class<?> getClazz()
return Object[].class;
}
}
+
+ public static final class NestedDataTypeStrategy implements TypeStrategy<Object>
+ {
+ public static final NestedDataTypeStrategy INSTANCE = new NestedDataTypeStrategy();
Review Comment:
nit, could put this at the top of `TypeStrategies` alongside the other static instances then would be `TypeStrategies.NESTED_DATA` or whatever
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add size check in DictionaryBuildingSingleValuedRowBasedKeySerdeHelper in putToKeyBuffer (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2408975841
##########
processing/src/main/java/org/apache/druid/segment/nested/StructuredData.java:
##########
@@ -133,16 +135,28 @@ private Number asNumber()
return (Number) value;
}
+ @SuppressWarnings("ReturnValueIgnored")
+ public int getSizeEstimate()
+ {
+ if (sizeEstimate < 0) {
+ hash.getAsLong(); // trigger hash computation which also sets sizeEstimate
+ }
+ Preconditions.checkState(sizeEstimate >= 0, "sizeEstimate not initialized");
+ return sizeEstimate;
+ }
+
@Override
public int compareTo(StructuredData o)
{
- if (this.equals(o)) {
+ if (this == o) {
Review Comment:
I thought this equal/compareTo is strange since it's not consistent. Equal doesn't handle nested array equal, if there's `Object[]` in a nested field, they don't equal, even though the compareTo returns equal since they're still the same data. But I've only seen places using compareTo so it might be fine.
Anyway I reverted the compareTo/equals part of change, removed the original hashCode method since it doesn't seem to make sense to keep two hash method. My fix is also not a perfect solution, since: 1. it sort of require object to be serialized even for the equal check now, 2. it still have the issue of hash collision.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add size check in DictionaryBuildingSingleValuedRowBasedKeySerdeHelper in putToKeyBuffer (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2396152630
##########
processing/src/main/java/org/apache/druid/segment/nested/StructuredData.java:
##########
@@ -133,16 +135,30 @@ private Number asNumber()
return (Number) value;
}
+ @SuppressWarnings("ReturnValueIgnored")
+ public int getSizeEstimate()
+ {
+ if (sizeEstimate < 0) {
+ hash.getAsLong(); // trigger hash computation which also sets sizeEstimate
+ }
+ Preconditions.checkState(sizeEstimate >= 0, "sizeEstimate not initialized");
+ return sizeEstimate;
+ }
+
@Override
public int compareTo(StructuredData o)
{
- if (this.equals(o)) {
+ if (this == o) {
return 0;
+ } else if (o == null) {
Review Comment:
it's true, this would be caught by all the null checks and null comparators so it'd never happen, removed it
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add size check in DictionaryBuildingSingleValuedRowBasedKeySerdeHelper in putToKeyBuffer (druid)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2396359836
##########
processing/src/main/java/org/apache/druid/segment/nested/StructuredData.java:
##########
@@ -170,7 +184,12 @@ public int compareTo(StructuredData o)
// finally compare hashes. there is a small chance of collisions for objects that are not equal but have the
// same hash, we could revisit this later if needed
- return Long.compare(hash.getAsLong(), o.hash.getAsLong());
+ int hashCompare = Long.compare(hash.getAsLong(), o.hash.getAsLong());
+ if (hashCompare != 0) {
+ return hashCompare;
+ }
+
+ return Integer.compare(getSizeEstimate(), o.getSizeEstimate());
Review Comment:
Using only (hash, sizeEstimate) as the final tie-breakers means two unequal values that collide on 64-bit hash and share the same serialized length will compare as 0 and thus be considered equal (see equals implementation), violating the general contract of equals/compareTo. To make equality logically correct, add a final fallback that compares the actual serialized byte arrays when hash and size are identical before returning 0.
##########
processing/src/main/java/org/apache/druid/segment/nested/StructuredData.java:
##########
@@ -183,20 +202,12 @@ public boolean equals(Object o)
return false;
}
StructuredData that = (StructuredData) o;
- if (value instanceof Object[] && that.value instanceof Object[]) {
- return Arrays.deepEquals((Object[]) value, (Object[]) that.value);
- }
- return Objects.equals(value, that.value);
+ // guarantees that equals is consistent with compareTo
+ return compareTo(that) == 0;
}
@Override
public int hashCode()
- {
- return Objects.hash(value);
- }
-
- // hashCode that relies on the object equality. Translates the hashcode to an integer as well
- public int equalityHash()
{
return Longs.hashCode(hash.getAsLong());
Review Comment:
[nitpick] hashCode truncates the 64-bit XXHash to 32 bits, increasing collision probability in hash-based collections for large datasets; consider mixing both high and low 32-bit parts (e.g., (int)(h ^ (h >>> 32))) to better distribute entropy.
```suggestion
long h = hash.getAsLong();
return (int)(h ^ (h >>> 32));
```
##########
processing/src/main/java/org/apache/druid/segment/nested/StructuredData.java:
##########
@@ -183,20 +202,12 @@ public boolean equals(Object o)
return false;
}
StructuredData that = (StructuredData) o;
- if (value instanceof Object[] && that.value instanceof Object[]) {
- return Arrays.deepEquals((Object[]) value, (Object[]) that.value);
- }
- return Objects.equals(value, that.value);
+ // guarantees that equals is consistent with compareTo
Review Comment:
[nitpick] Routing equals through compareTo forces the full ordering path (including hash/size computations and potential serialization) for simple equality checks, adding overhead to hash-based collections; introduce a fast path (e.g., identity, null, primitive/String direct comparison, then serialized-bytes equality) before invoking ordering logic.
```suggestion
// Fast path for null
if (isNull() && that.isNull()) {
return true;
} else if (isNull() || that.isNull()) {
return false;
}
// Fast path for String
if (isString() && that.isString()) {
return asString().equals(that.asString());
}
// Fast path for Number
if (isNumber() && that.isNumber()) {
// Use Double.compare for equality of numbers
return Double.compare(asNumber().doubleValue(), that.asNumber().doubleValue()) == 0;
}
// For complex objects, compare hash and size estimate first
if (hash.getAsLong() != that.hash.getAsLong()) {
return false;
}
if (getSizeEstimate() != that.getSizeEstimate()) {
return false;
}
// Fallback to compareTo for final check (should be rare)
```
##########
processing/src/main/java/org/apache/druid/segment/nested/StructuredData.java:
##########
@@ -183,20 +202,12 @@ public boolean equals(Object o)
return false;
}
StructuredData that = (StructuredData) o;
- if (value instanceof Object[] && that.value instanceof Object[]) {
- return Arrays.deepEquals((Object[]) value, (Object[]) that.value);
- }
- return Objects.equals(value, that.value);
+ // guarantees that equals is consistent with compareTo
+ return compareTo(that) == 0;
Review Comment:
equals delegates entirely to compareTo, which (due to the current hash/size tie-breaker) can declare different logical values equal on hash collision plus equal length, creating a possibility of incorrect equality semantics; equals should instead directly compare underlying values (e.g., by serialized bytes or structured value comparison) and only use compareTo for ordering. Recommend implementing equals via structural/byte-array equality and letting compareTo remain a total ordering.
```suggestion
return java.util.Objects.equals(this.value, that.value);
```
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add size check in DictionaryBuildingSingleValuedRowBasedKeySerdeHelper in putToKeyBuffer (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2411323590
##########
processing/src/test/java/org/apache/druid/segment/nested/StructuredDataTest.java:
##########
@@ -106,10 +105,4 @@ public void testCompareToWithDifferentJSONOrder()
Assert.assertEquals(1, sd1.compareTo(sd2));
Assert.assertEquals(0, sd0.compareTo(sd1));
}
-
- @Test
- public void testEqualsAndHashcode()
- {
- EqualsVerifier.forClass(StructuredData.class).withIgnoredFields("hashInitialized", "hashValue", "hash").usingGetClass().verify();
- }
Review Comment:
something with `EqualsVerifier`, it doesn't allow the `hashCode` being delegated to an abstract method, i'm not quite sure, but looks like it doesnt like the hash longSupplier
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add size check in DictionaryBuildingSingleValuedRowBasedKeySerdeHelper in putToKeyBuffer (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei merged PR #18541:
URL: https://github.com/apache/druid/pull/18541
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add dictionary size estimate in `DictionaryBuildingSingleValuedRowBasedKeySerdeHelper` (druid)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2357220533
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java:
##########
@@ -1619,6 +1627,11 @@ public boolean putToKeyBuffer(RowBasedKey key, int idx)
final Object obj = key.getKey()[idx];
int id = getReverseDictionary().getInt(obj);
if (id == DimensionDictionary.ABSENT_VALUE_ID) {
+ int size = estimatedKeySize(key.getKey());
+ if (currentEstimatedSize + size > maxDictionarySize) {
+ return false;
+ }
+ currentEstimatedSize += size;
Review Comment:
The size estimation is calculated for the entire key array, but only the current dimension (at index `idx`) should be added to the dictionary. This leads to incorrect size accounting as it includes sizes of dimensions that may already be in the dictionary.
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java:
##########
@@ -1692,6 +1707,24 @@ private void validateColumnType(TypeSignature<ValueType> columnType)
}
}
+ @Override
+ int estimatedKeySize(Object[] key)
+ {
+ int size = 0;
+ for (Object obj : key) {
+ if (obj instanceof StructuredData) {
+ Object o = ((StructuredData) obj).getValue();
+ if (o instanceof String) {
+ size += (int) estimateStringKeySize((String) o);
+ } else {
+ // fall back to string representation for other types, this might be under-estimating for map
+ size += (int) estimateStringKeySize(String.valueOf(o));
+ }
Review Comment:
The method only processes StructuredData objects but ignores other types in the key array. This could lead to underestimating the total size when the key contains non-StructuredData objects.
```suggestion
}
} else if (obj != null) {
// For non-StructuredData objects, estimate size using their string representation
size += (int) estimateStringKeySize(String.valueOf(obj));
```
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add size check in DictionaryBuildingSingleValuedRowBasedKeySerdeHelper in putToKeyBuffer (druid)
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2373703256
##########
processing/src/main/java/org/apache/druid/segment/nested/StructuredData.java:
##########
@@ -133,16 +135,29 @@
return (Number) value;
}
+ public int getSizeEstimate()
+ {
+ if (sizeEstimate < 0) {
+ var unused = hash.getAsLong(); // trigger hash computation which also sets sizeEstimate
Review Comment:
## Unread local variable
Variable 'long unused' is never read.
[Show more details](https://github.com/apache/druid/security/code-scanning/10406)
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add dictionary size estimate in `DictionaryBuildingSingleValuedRowBasedKeySerdeHelper` (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2364510682
##########
processing/src/main/java/org/apache/druid/query/groupby/epinephelinae/RowBasedGrouperHelper.java:
##########
@@ -1619,6 +1627,11 @@ public boolean putToKeyBuffer(RowBasedKey key, int idx)
final Object obj = key.getKey()[idx];
int id = getReverseDictionary().getInt(obj);
if (id == DimensionDictionary.ABSENT_VALUE_ID) {
+ int size = estimatedKeySize(key.getKey());
+ if (currentEstimatedSize + size > maxDictionarySize) {
+ return false;
+ }
+ currentEstimatedSize += size;
Review Comment:
this comment is correct, it should be `estimateKeySize(obj)`, not the whole grouping key array
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add size check in DictionaryBuildingSingleValuedRowBasedKeySerdeHelper in putToKeyBuffer (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2408979640
##########
processing/src/main/java/org/apache/druid/segment/column/TypeStrategies.java:
##########
@@ -697,4 +704,103 @@ public Class<?> getClazz()
return Object[].class;
}
}
+
+ public static final class NestedDataTypeStrategy implements TypeStrategy<Object>
+ {
+ public static final NestedDataTypeStrategy INSTANCE = new NestedDataTypeStrategy();
Review Comment:
updated
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add size check in DictionaryBuildingSingleValuedRowBasedKeySerdeHelper in putToKeyBuffer (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2389309357
##########
processing/src/main/java/org/apache/druid/segment/nested/StructuredData.java:
##########
@@ -133,16 +135,30 @@ private Number asNumber()
return (Number) value;
}
+ @SuppressWarnings("ReturnValueIgnored")
+ public int getSizeEstimate()
+ {
+ if (sizeEstimate < 0) {
+ hash.getAsLong(); // trigger hash computation which also sets sizeEstimate
+ }
+ Preconditions.checkState(sizeEstimate >= 0, "sizeEstimate not initialized");
+ return sizeEstimate;
+ }
+
@Override
public int compareTo(StructuredData o)
{
- if (this.equals(o)) {
+ if (this == o) {
return 0;
+ } else if (o == null) {
Review Comment:
does this happen? Is fine since is harmless to be here, but i think most users of this would be dealing in `StructuredData` where a null value is still wrapped (so probably shouldn't appear here much in practice)
##########
processing/src/main/java/org/apache/druid/segment/column/TypeStrategies.java:
##########
@@ -697,4 +701,77 @@ public Class<?> getClazz()
return Object[].class;
}
}
+
+ public static final class JsonTypeStrategy implements TypeStrategy
Review Comment:
this should probably be named `NestedDataTypeStrategy` to be consistent with other internal naming
##########
processing/src/main/java/org/apache/druid/segment/column/TypeStrategies.java:
##########
@@ -697,4 +701,77 @@ public Class<?> getClazz()
return Object[].class;
}
}
+
+ public static final class JsonTypeStrategy implements TypeStrategy
+ {
+ private final ObjectStrategy objectStrategy;
Review Comment:
nit: it feels a bit off to me to have to make an `ObjectStrategy` to implement the `TypeStrategy`, where ideally I think I would expect both the nested column `ObjectStrategy` and `TypeStrategy` to be implemented such that they can just share some common methods instead of one relying on the other. `ObjectStrategyComplexTypeStrategy` was a sort of backwards compatible fallback to make it so that we could fill in `TypeStrategy` from pre-existing `ObjectStrategy`.
Not a big deal though
##########
processing/src/main/java/org/apache/druid/segment/nested/NestedDataComplexTypeSerde.java:
##########
@@ -124,7 +123,7 @@ public Object fromByteBuffer(ByteBuffer buffer, int numBytes)
@Override
public byte[] toBytes(@Nullable Object val)
{
- return serializeToBytes(val);
+ return serializeToBytes(StructuredData.wrap(val));
Review Comment:
what is this for? the `StructuredData` [serializer is just going to unwrap this when serializing]( https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/nested/StructuredDataJsonSerializer.java#L29)
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add size check in DictionaryBuildingSingleValuedRowBasedKeySerdeHelper in putToKeyBuffer (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2396281851
##########
processing/src/main/java/org/apache/druid/segment/column/TypeStrategies.java:
##########
@@ -697,4 +701,77 @@ public Class<?> getClazz()
return Object[].class;
}
}
+
+ public static final class JsonTypeStrategy implements TypeStrategy
Review Comment:
updated
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add size check in DictionaryBuildingSingleValuedRowBasedKeySerdeHelper in putToKeyBuffer (druid)
Posted by "cecemei (via GitHub)" <gi...@apache.org>.
cecemei commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2396282529
##########
processing/src/main/java/org/apache/druid/segment/column/TypeStrategies.java:
##########
@@ -697,4 +701,77 @@ public Class<?> getClazz()
return Object[].class;
}
}
+
+ public static final class JsonTypeStrategy implements TypeStrategy
+ {
+ private final ObjectStrategy objectStrategy;
Review Comment:
moved all functions to TypeStrategy, looks better for encapsulation!
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add size check in DictionaryBuildingSingleValuedRowBasedKeySerdeHelper in putToKeyBuffer (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2411314025
##########
processing/src/main/java/org/apache/druid/segment/nested/StructuredData.java:
##########
@@ -133,16 +135,28 @@ private Number asNumber()
return (Number) value;
}
+ @SuppressWarnings("ReturnValueIgnored")
+ public int getSizeEstimate()
+ {
+ if (sizeEstimate < 0) {
+ hash.getAsLong(); // trigger hash computation which also sets sizeEstimate
+ }
+ Preconditions.checkState(sizeEstimate >= 0, "sizeEstimate not initialized");
+ return sizeEstimate;
+ }
+
@Override
public int compareTo(StructuredData o)
{
- if (this.equals(o)) {
+ if (this == o) {
Review Comment:
ah yea, it totally is strange, and i wasn't necessarily encouraging reverting it, just was curious if was directly related or not. I think it would be ok to consider making these all consistent with each other, but we can do that in a separate PR since it also doesn't really matter much either way since compareTo is the main thing that is used by stuff.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add size check in DictionaryBuildingSingleValuedRowBasedKeySerdeHelper in putToKeyBuffer (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #18541:
URL: https://github.com/apache/druid/pull/18541#discussion_r2411303007
##########
processing/src/test/java/org/apache/druid/segment/nested/StructuredDataTest.java:
##########
@@ -106,10 +105,4 @@ public void testCompareToWithDifferentJSONOrder()
Assert.assertEquals(1, sd1.compareTo(sd2));
Assert.assertEquals(0, sd0.compareTo(sd1));
}
-
- @Test
- public void testEqualsAndHashcode()
- {
- EqualsVerifier.forClass(StructuredData.class).withIgnoredFields("hashInitialized", "hashValue", "hash").usingGetClass().verify();
- }
Review Comment:
why did we need to remove this?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org