Skip to content
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.

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

Add a length limit of 512 to the properties stored in the segment metadata #6008

Merged

Conversation

Jackie-Jiang
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang commented Sep 11, 2020

Description

Prevent storing very long values into the segment metadata. This could happen for text column, bytes column for serialized objects, or blob store use cases (not recommended but supported).

@codecov-commenter
Copy link

Codecov Report

Merging #6008 into master will decrease coverage by 20.10%.
The diff coverage is 49.63%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #6008       +/-   ##
===========================================
- Coverage   66.44%   46.34%   -20.11%     
===========================================
  Files        1075     1180      +105     
  Lines       54773    55889     +1116     
  Branches     8168     8134       -34     
===========================================
- Hits        36396    25899    -10497     
- Misses      15700    27869    +12169     
+ Partials     2677     2121      -556     
Flag Coverage Δ
#integration 46.34% <49.63%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ot/broker/broker/AllowAllAccessControlFactory.java 100.00% <ø> (ø)
.../helix/BrokerUserDefinedMessageHandlerFactory.java 52.83% <0.00%> (-13.84%) ⬇️
...org/apache/pinot/broker/queryquota/HitCounter.java 0.00% <0.00%> (-100.00%) ⬇️
...che/pinot/broker/queryquota/MaxHitRateTracker.java 0.00% <0.00%> (ø)
...ache/pinot/broker/queryquota/QueryQuotaEntity.java 0.00% <0.00%> (-50.00%) ⬇️
...ava/org/apache/pinot/client/AbstractResultSet.java 26.66% <0.00%> (-30.48%) ⬇️
.../main/java/org/apache/pinot/client/Connection.java 22.22% <0.00%> (-26.62%) ⬇️
.../org/apache/pinot/client/ResultTableResultSet.java 24.00% <0.00%> (-10.29%) ⬇️
.../apache/pinot/common/function/FunctionInvoker.java 61.36% <ø> (ø)
...apache/pinot/common/function/FunctionRegistry.java 78.78% <ø> (ø)
... and 1130 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11fd62b...5593656. Read the comment docs.

@@ -78,6 +78,9 @@
public class SegmentColumnarIndexCreator implements SegmentCreator {
// TODO Refactor class name to match interface name
private static final Logger LOGGER = LoggerFactory.getLogger(SegmentColumnarIndexCreator.class);
// Allow at most 512 characters for the metadata property
private static final int METADATA_PROPERTY_LENGTH_LIMIT = 512;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall this align with FieldSpec.DEFAULT_MAX_LENGTH?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are independent, where the maxLength is configurable in the FieldSpec to allow ingesting long strings, but the property length limit is fixed. Also, the property length limit also applies to the BYTES column.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks

@Jackie-Jiang Jackie-Jiang merged commit b2df8ea into apache:master Sep 15, 2020
@Jackie-Jiang Jackie-Jiang deleted the metadata_property_length_limit branch September 15, 2020 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants