add max length support in schema builder #6112

deemoliu · 2020-10-06T18:13:02Z

Description

Add one more API to set maxLength in schemaBuilder. #6106

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

New configuration options
Deprecation of configurations
Signature changes to public methods/interfaces
New plugins added or old plugins removed

Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release.

If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text

Documentation

If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document

codecov-io · 2020-10-06T21:29:12Z

Codecov Report

Merging #6112 into master will increase coverage by 6.42%.
The diff coverage is 59.69%.

@@            Coverage Diff             @@
##           master    #6112      +/-   ##
==========================================
+ Coverage   66.44%   72.87%   +6.42%     
==========================================
  Files        1075     1224     +149     
  Lines       54773    57796    +3023     
  Branches     8168     8524     +356     
==========================================
+ Hits        36396    42116    +5720     
+ Misses      15700    12917    -2783     
- Partials     2677     2763      +86

Flag	Coverage Δ
#integration	`45.37% <48.75%> (?)`
#unittests	`64.01% <38.00%> (?)`

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%)`	⬇️
...ava/org/apache/pinot/client/AbstractResultSet.java	`53.33% <0.00%> (-3.81%)`	⬇️
.../main/java/org/apache/pinot/client/Connection.java	`44.44% <0.00%> (-4.40%)`	⬇️
.../org/apache/pinot/client/ResultTableResultSet.java	`24.00% <0.00%> (-10.29%)`	⬇️
...not/common/assignment/InstancePartitionsUtils.java	`78.57% <ø> (+5.40%)`	⬆️
.../apache/pinot/common/exception/QueryException.java	`90.27% <ø> (+5.55%)`	⬆️
...pinot/common/function/AggregationFunctionType.java	`98.27% <ø> (-1.73%)`	⬇️
.../pinot/common/function/DateTimePatternHandler.java	`83.33% <ø> (ø)`
...ot/common/function/FunctionDefinitionRegistry.java	`88.88% <ø> (+44.44%)`	⬆️
... and 973 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 e1ab421...0487101. Read the comment docs.

Jackie-Jiang

LGTM with minor comments

Jackie-Jiang · 2020-10-07T00:40:26Z

pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java

+     * Add single value dimensionFieldSpec with maxLength and a defaultNullValue
+     */
+    public SchemaBuilder addSingleValueDimension(String dimensionName, DataType dataType, int maxLength,
+                                                 Object defaultNullValue) {


Please reformat this

Gotcha, thanks @Jackie-Jiang for code review.

Jackie-Jiang · 2020-10-07T00:40:44Z

pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java

+     * Add multi value dimensionFieldSpec with maxLength and a defaultNullValue
+     */
+    public SchemaBuilder addMultiValueDimension(String dimensionName, DataType dataType, int maxLength,
+                                                Object defaultNullValue) {


Reformat this

Gotcha, thanks for code review.

mayankshriv · 2020-10-07T16:01:45Z

pinot-common/src/test/java/org/apache/pinot/common/data/FieldSpecTest.java

+    fieldSpec1.setName("svDimension");
+    fieldSpec1.setDataType(BOOLEAN);
+    fieldSpec1.setDefaultNullValue(false);
+    fieldSpec1.setMaxLength(20000);


Why do fixed length data types (BOOLEAN/int/float/etc) need maxLength? If not, then we should not allow setting max-length for those and only apply to variable length data types (String/Bytes).

Good point. The maxLength field only applies to STRING field right now (not even BYTES)

Thanks @mayankshriv @Jackie-Jiang for review. I will fix this test. Additional to that, do you suggest me to enforce configurable maxLength on String fields only?
Is it possible that Pinot will support maxLength on other variable length data types in the future?

In our use case, we only allow user to set maxLength on String columns.

@Jackie-Jiang How about multi-valued fixed length data types? E.g. Does maxLength applied to multi-value long fields?

@deemoliu The maxLength should only be applied to String field. The method names in Schema.java should reflect this as well. i.e., the method name should be addStringMetric().

@chenboat @Jackie-Jiang thanks for the code review. I added precondition in Schema.java for dimension columns. Regarding metrics column, does METRIC column support String column?

I think it should be supported according to https://docs.pinot.apache.org/configuration-reference/schema#metricfieldspecs

However in the schema.java it weren't been supported.
https://github.com/apache/incubator-pinot/blob/master/pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java#L447

It looks like the doc is not in sync with the code. The code is usually the source of truth. For this PR, I think the update to METRICS column can be skipped.

I see. Then I will remove the code related to "metrics with max length" (since maxlength ony applied to String and metrics columns cannot be string type) and only keep "dimension with max length".

chenboat · 2020-10-10T04:12:09Z

pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java

+    /**
+     * Add single value dimensionFieldSpec with maxLength and a defaultNullValue
+     */
+    public SchemaBuilder addSingleValueDimension(String dimensionName, DataType dataType, int maxLength,


the maxLength should be applicable only to String type only as discussed earlier. You can add a Precondition Check here to make sure dataType is String.

gotcha, added precondition check. thanks for review.

chenboat requested review from chenboat and Jackie-Jiang October 6, 2020 18:21

deemoliu mentioned this pull request Oct 6, 2020

Support customized maxLength for fields in schemaBuilder #6106

Closed

Jackie-Jiang approved these changes Oct 7, 2020

View reviewed changes

mayankshriv reviewed Oct 7, 2020

View reviewed changes

chenboat reviewed Oct 10, 2020

View reviewed changes

chenboat approved these changes Oct 13, 2020

View reviewed changes

deemoliu added 5 commits October 13, 2020 15:26

add max length support in schema builder

9d38038

fix the default value of metric fields in unit tests

f507cdb

fix a unit test to aviod maxlength on boolean fields, reformat

581bc6c

add precondition check for add fields with max length.

d441387

remove add string metric with max length

0487101

deemoliu requested a review from mayankshriv October 14, 2020 00:05

Jackie-Jiang merged commit 65be8bd into apache:master Oct 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add max length support in schema builder #6112

add max length support in schema builder #6112

deemoliu commented Oct 6, 2020 •

edited

Loading

codecov-io commented Oct 6, 2020 •

edited

Loading

Jackie-Jiang left a comment

Jackie-Jiang Oct 7, 2020

deemoliu Oct 8, 2020

Jackie-Jiang Oct 7, 2020

deemoliu Oct 8, 2020

mayankshriv Oct 7, 2020

Jackie-Jiang Oct 7, 2020

deemoliu Oct 8, 2020

deemoliu Oct 8, 2020

chenboat Oct 10, 2020

deemoliu Oct 12, 2020

chenboat Oct 12, 2020

deemoliu Oct 12, 2020

chenboat Oct 10, 2020

deemoliu Oct 12, 2020

add max length support in schema builder #6112

add max length support in schema builder #6112

Conversation

deemoliu commented Oct 6, 2020 • edited Loading

Description

Upgrade Notes

Release Notes

Documentation

codecov-io commented Oct 6, 2020 • edited Loading

Codecov Report

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deemoliu commented Oct 6, 2020 •

edited

Loading

codecov-io commented Oct 6, 2020 •

edited

Loading