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 2024/08/28 13:23:56 UTC
[PR] Support ComplexFieldSpec in Schema and column metadata [pinot]
xiangfu0 opened a new pull request, #13905:
URL: https://github.com/apache/pinot/pull/13905
- Fix all the ComplexFieldSpec serde in Schema class
- Fix ComplexFieldSpec usage in ColumnMetadata
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1735188842
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -606,15 +598,10 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
}
}
- // datetime field
- if (fieldSpec.getFieldType().equals(FieldType.DATE_TIME)) {
- DateTimeFieldSpec dateTimeFieldSpec = (DateTimeFieldSpec) fieldSpec;
- properties.setProperty(getKeyFor(column, DATETIME_FORMAT), dateTimeFieldSpec.getFormat());
- properties.setProperty(getKeyFor(column, DATETIME_GRANULARITY), dateTimeFieldSpec.getGranularity());
- }
-
+ addFieldSpec(properties, column, fieldSpec);
+ DataType dataType = fieldSpec.getDataType();
// NOTE: Min/max could be null for real-time aggregate metrics.
- if (totalDocs > 0) {
+ if ((fieldSpec.getFieldType() != FieldType.COMPLEX) && (totalDocs > 0)) {
Review Comment:
The refactor code here changes the ordering of each column props, which changes the CRC.
@Jackie-Jiang
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1735456588
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -475,6 +477,7 @@ private void writeMetadata()
properties.setProperty(DIMENSIONS, _config.getDimensions());
properties.setProperty(METRICS, _config.getMetrics());
properties.setProperty(DATETIME_COLUMNS, _config.getDateTimeColumnNames());
+ properties.setProperty(COMPLEX_COLUMNS, _config.getComplexColumnNames());
Review Comment:
It's not persisting if value is empty.
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1737790699
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -613,8 +616,19 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
properties.setProperty(getKeyFor(column, DATETIME_GRANULARITY), dateTimeFieldSpec.getGranularity());
}
+ // complex field
+ if (fieldSpec.getFieldType().equals(FieldType.COMPLEX)) {
+ ComplexFieldSpec complexFieldSpec = (ComplexFieldSpec) fieldSpec;
+ properties.setProperty(getKeyFor(column, COMPLEX_CHILD_FIELD_NAMES),
+ new ArrayList<>(complexFieldSpec.getChildFieldSpecs().keySet()));
+ for (Map.Entry<String, FieldSpec> entry : complexFieldSpec.getChildFieldSpecs().entrySet()) {
+ addFieldSpec(properties, ComplexFieldSpec.getFullChildName(column, entry.getKey()), entry.getValue());
+ }
+ }
+
+ addFieldSpec(properties, column, fieldSpec);
Review Comment:
removed. Should only addFieldSpec inside the ComplexFieldSpec
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1735183987
##########
pinot-core/src/test/java/org/apache/pinot/core/util/CrcUtilsTest.java:
##########
@@ -38,8 +38,8 @@
public class CrcUtilsTest {
private static final File INDEX_DIR = new File(FileUtils.getTempDirectory(), "CrcUtilsTest");
private static final String AVRO_DATA = "data/test_data-mv.avro";
- private static final long EXPECTED_V1_CRC = 2708456273L;
- private static final long EXPECTED_V3_CRC = 2796149869L;
+ private static final long EXPECTED_V1_CRC = 3087108662L;
Review Comment:
Ideally we want to keep it the same, or we might end up downloading segments because different versions of Pinot generating different crc
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1735400036
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -475,6 +477,7 @@ private void writeMetadata()
properties.setProperty(DIMENSIONS, _config.getDimensions());
properties.setProperty(METRICS, _config.getMetrics());
properties.setProperty(DATETIME_COLUMNS, _config.getDateTimeColumnNames());
+ properties.setProperty(COMPLEX_COLUMNS, _config.getComplexColumnNames());
Review Comment:
Hmm, why did the test pass then
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1736752128
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -613,8 +616,19 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
properties.setProperty(getKeyFor(column, DATETIME_GRANULARITY), dateTimeFieldSpec.getGranularity());
}
+ // complex field
+ if (fieldSpec.getFieldType().equals(FieldType.COMPLEX)) {
Review Comment:
(nit) Same for other enum checks
```suggestion
if (fieldSpec.getFieldType() == FieldType.COMPLEX) {
```
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -633,6 +647,55 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
}
}
+ /**
+ * In order to persist complex field metadata, we need to recursively add child field specs
+ * So, each complex field spec will have a property for its child field names and each child field will have its
+ * own properties of the detailed field spec.
+ * E.g. a COMPLEX type `intMap` of Map<String, Integer> has 2 child fields:
+ * - key in STRING type and value in INT type.
+ * Then we will have the following properties to define a COMPLEX field:
+ * column.intMap.childFieldNames = [key, value]
+ * column.intMap$$key.columnType = DIMENSION
+ * column.intMap$$key.dataType = STRING
+ * column.intMap$$key.isSingleValued = true
+ * column.intMap$$value.columnType = DIMENSION
+ * column.intMap$$value.dataType = INT
+ * column.intMap$$value.isSingleValued = true
+ */
+ public static void addFieldSpec(PropertiesConfiguration properties, String column, FieldSpec fieldSpec) {
Review Comment:
Do we have enough info stored to load the index? In order to load dictionary, I believe we need to store cardinality, bits per element and other dictionary related info
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -633,6 +647,55 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
}
}
+ /**
+ * In order to persist complex field metadata, we need to recursively add child field specs
+ * So, each complex field spec will have a property for its child field names and each child field will have its
+ * own properties of the detailed field spec.
+ * E.g. a COMPLEX type `intMap` of Map<String, Integer> has 2 child fields:
+ * - key in STRING type and value in INT type.
+ * Then we will have the following properties to define a COMPLEX field:
+ * column.intMap.childFieldNames = [key, value]
+ * column.intMap$$key.columnType = DIMENSION
+ * column.intMap$$key.dataType = STRING
+ * column.intMap$$key.isSingleValued = true
+ * column.intMap$$value.columnType = DIMENSION
+ * column.intMap$$value.dataType = INT
+ * column.intMap$$value.isSingleValued = true
+ */
+ public static void addFieldSpec(PropertiesConfiguration properties, String column, FieldSpec fieldSpec) {
+ properties.setProperty(getKeyFor(column, COLUMN_TYPE), String.valueOf(fieldSpec.getFieldType()));
+ if (!column.equals(fieldSpec.getName())) {
+ properties.setProperty(getKeyFor(column, COLUMN_NAME), String.valueOf(fieldSpec.getName()));
Review Comment:
Do we ever allow this? Will complex type has a different field name under the key?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -613,8 +616,19 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
properties.setProperty(getKeyFor(column, DATETIME_GRANULARITY), dateTimeFieldSpec.getGranularity());
}
+ // complex field
+ if (fieldSpec.getFieldType().equals(FieldType.COMPLEX)) {
+ ComplexFieldSpec complexFieldSpec = (ComplexFieldSpec) fieldSpec;
+ properties.setProperty(getKeyFor(column, COMPLEX_CHILD_FIELD_NAMES),
+ new ArrayList<>(complexFieldSpec.getChildFieldSpecs().keySet()));
+ for (Map.Entry<String, FieldSpec> entry : complexFieldSpec.getChildFieldSpecs().entrySet()) {
+ addFieldSpec(properties, ComplexFieldSpec.getFullChildName(column, entry.getKey()), entry.getValue());
+ }
+ }
+
+ addFieldSpec(properties, column, fieldSpec);
Review Comment:
I'm a little bit confused. Are we adding the same properties again?
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1735210140
##########
pinot-core/src/test/java/org/apache/pinot/core/util/CrcUtilsTest.java:
##########
@@ -38,8 +38,8 @@
public class CrcUtilsTest {
private static final File INDEX_DIR = new File(FileUtils.getTempDirectory(), "CrcUtilsTest");
private static final String AVRO_DATA = "data/test_data-mv.avro";
- private static final long EXPECTED_V1_CRC = 2708456273L;
- private static final long EXPECTED_V3_CRC = 2796149869L;
+ private static final long EXPECTED_V1_CRC = 3087108662L;
Review Comment:
Ok, reverted the code then, only use `addFieldSpec` for adding child FieldSpecs.
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1737790699
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -613,8 +616,19 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
properties.setProperty(getKeyFor(column, DATETIME_GRANULARITY), dateTimeFieldSpec.getGranularity());
}
+ // complex field
+ if (fieldSpec.getFieldType().equals(FieldType.COMPLEX)) {
+ ComplexFieldSpec complexFieldSpec = (ComplexFieldSpec) fieldSpec;
+ properties.setProperty(getKeyFor(column, COMPLEX_CHILD_FIELD_NAMES),
+ new ArrayList<>(complexFieldSpec.getChildFieldSpecs().keySet()));
+ for (Map.Entry<String, FieldSpec> entry : complexFieldSpec.getChildFieldSpecs().entrySet()) {
+ addFieldSpec(properties, ComplexFieldSpec.getFullChildName(column, entry.getKey()), entry.getValue());
+ }
+ }
+
+ addFieldSpec(properties, column, fieldSpec);
Review Comment:
removed.
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1735176926
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/ComplexFieldSpec.java:
##########
@@ -60,18 +65,24 @@ public ComplexFieldSpec() {
_childFieldSpecs = new HashMap<>();
}
+ @Deprecated
Review Comment:
removed.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/ComplexFieldSpec.java:
##########
@@ -60,18 +65,24 @@ public ComplexFieldSpec() {
_childFieldSpecs = new HashMap<>();
}
+ @Deprecated
public ComplexFieldSpec(@Nonnull String name, DataType dataType, boolean isSingleValueField) {
+ this(name, dataType, isSingleValueField, new HashMap<>());
+ }
+
+ public ComplexFieldSpec(@Nonnull String name, DataType dataType, boolean isSingleValueField,
Review Comment:
done
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1735157736
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/ComplexFieldSpec.java:
##########
@@ -60,18 +65,24 @@ public ComplexFieldSpec() {
_childFieldSpecs = new HashMap<>();
}
+ @Deprecated
public ComplexFieldSpec(@Nonnull String name, DataType dataType, boolean isSingleValueField) {
+ this(name, dataType, isSingleValueField, new HashMap<>());
+ }
+
+ public ComplexFieldSpec(@Nonnull String name, DataType dataType, boolean isSingleValueField,
Review Comment:
Let's remove all `@Nonnull` annotations. All objects are non-null here
##########
pinot-core/src/test/java/org/apache/pinot/core/util/CrcUtilsTest.java:
##########
@@ -38,8 +38,8 @@
public class CrcUtilsTest {
private static final File INDEX_DIR = new File(FileUtils.getTempDirectory(), "CrcUtilsTest");
private static final String AVRO_DATA = "data/test_data-mv.avro";
- private static final long EXPECTED_V1_CRC = 2708456273L;
- private static final long EXPECTED_V3_CRC = 2796149869L;
+ private static final long EXPECTED_V1_CRC = 3087108662L;
Review Comment:
Why does this change?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/ComplexFieldSpec.java:
##########
@@ -87,6 +98,58 @@ public FieldType getFieldType() {
@Override
public String toString() {
- return "field type: COMPLEX, field name: " + _name + ", root data type: " + _dataType;
+ return "field type: COMPLEX, field name: " + _name + ", root data type: " + _dataType + ", child field specs: "
+ + _childFieldSpecs;
+ }
+
+ public static class MapFieldSpec {
+ private final String _fieldName;
+ private final FieldSpec _keyFieldSpec;
+ private final FieldSpec _valueFieldSpec;
+
+ private MapFieldSpec(ComplexFieldSpec complexFieldSpec) {
+ Preconditions.checkState(complexFieldSpec.getChildFieldSpecs().containsKey(KEY_FIELD),
+ "Missing 'key' in the 'childFieldSpec'");
+ Preconditions.checkState(complexFieldSpec.getChildFieldSpecs().containsKey(VALUE_FIELD),
+ "Missing 'value' in the 'childFieldSpec'");
+ _keyFieldSpec = complexFieldSpec.getChildFieldSpec(KEY_FIELD);
+ _valueFieldSpec = complexFieldSpec.getChildFieldSpec(VALUE_FIELD);
+ _fieldName = complexFieldSpec.getName();
+ }
+
+ public String getFieldName() {
+ return _fieldName;
+ }
+
+ public FieldSpec getKeyFieldSpec() {
+ return _keyFieldSpec;
+ }
+
+ public FieldSpec getValueFieldSpec() {
+ return _valueFieldSpec;
+ }
+ }
+
+ public static MapFieldSpec fromMapFieldSpec(ComplexFieldSpec complexFieldSpec) {
+ return new MapFieldSpec(complexFieldSpec);
+ }
+
+ public static ComplexFieldSpec fromMapFieldSpec(MapFieldSpec mapFieldSpec) {
+ return new ComplexFieldSpec(mapFieldSpec.getFieldName(), DataType.MAP, true,
+ Map.of(KEY_FIELD, mapFieldSpec.getKeyFieldSpec(), VALUE_FIELD, mapFieldSpec.getValueFieldSpec()));
+ }
+
+ public static String getFullChildName(String... columns) {
Review Comment:
What is the definition of full child name? Some javadoc will be helpful
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/ComplexFieldSpec.java:
##########
@@ -60,18 +65,24 @@ public ComplexFieldSpec() {
_childFieldSpecs = new HashMap<>();
}
+ @Deprecated
Review Comment:
I don't think this is used in production, so no need to keep backward compatibility
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/ComplexFieldSpec.java:
##########
@@ -87,6 +98,58 @@ public FieldType getFieldType() {
@Override
public String toString() {
- return "field type: COMPLEX, field name: " + _name + ", root data type: " + _dataType;
+ return "field type: COMPLEX, field name: " + _name + ", root data type: " + _dataType + ", child field specs: "
+ + _childFieldSpecs;
+ }
+
+ public static class MapFieldSpec {
+ private final String _fieldName;
+ private final FieldSpec _keyFieldSpec;
+ private final FieldSpec _valueFieldSpec;
+
+ private MapFieldSpec(ComplexFieldSpec complexFieldSpec) {
+ Preconditions.checkState(complexFieldSpec.getChildFieldSpecs().containsKey(KEY_FIELD),
+ "Missing 'key' in the 'childFieldSpec'");
+ Preconditions.checkState(complexFieldSpec.getChildFieldSpecs().containsKey(VALUE_FIELD),
+ "Missing 'value' in the 'childFieldSpec'");
+ _keyFieldSpec = complexFieldSpec.getChildFieldSpec(KEY_FIELD);
+ _valueFieldSpec = complexFieldSpec.getChildFieldSpec(VALUE_FIELD);
+ _fieldName = complexFieldSpec.getName();
+ }
+
+ public String getFieldName() {
+ return _fieldName;
+ }
+
+ public FieldSpec getKeyFieldSpec() {
+ return _keyFieldSpec;
+ }
+
+ public FieldSpec getValueFieldSpec() {
+ return _valueFieldSpec;
+ }
+ }
+
+ public static MapFieldSpec fromMapFieldSpec(ComplexFieldSpec complexFieldSpec) {
Review Comment:
`toMapFieldSpec`?
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #13905:
URL: https://github.com/apache/pinot/pull/13905#issuecomment-2315936004
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/13905?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `143 lines` in your changes missing coverage. Please review.
> Project coverage is 0.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`f0582ee`)](https://app.codecov.io/gh/apache/pinot/commit/f0582ee51b692b92d694603e216fab0d420f1a91?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 953 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/13905?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/index/metadata/ColumnMetadataImpl.java](https://app.codecov.io/gh/apache/pinot/pull/13905?src=pr&el=tree&filepath=pinot-segment-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Fspi%2Findex%2Fmetadata%2FColumnMetadataImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L21ldGFkYXRhL0NvbHVtbk1ldGFkYXRhSW1wbC5qYXZh) | 0.00% | [50 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13905?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...va/org/apache/pinot/spi/data/ComplexFieldSpec.java](https://app.codecov.io/gh/apache/pinot/pull/13905?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fdata%2FComplexFieldSpec.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9Db21wbGV4RmllbGRTcGVjLmphdmE=) | 0.00% | [28 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13905?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ment/creator/impl/SegmentColumnarIndexCreator.java](https://app.codecov.io/gh/apache/pinot/pull/13905?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Fsegment%2Fcreator%2Fimpl%2FSegmentColumnarIndexCreator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50Q29sdW1uYXJJbmRleENyZWF0b3IuamF2YQ==) | 0.00% | [25 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13905?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...main/java/org/apache/pinot/spi/data/FieldSpec.java](https://app.codecov.io/gh/apache/pinot/pull/13905?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fdata%2FFieldSpec.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9GaWVsZFNwZWMuamF2YQ==) | 0.00% | [19 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13905?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...rc/main/java/org/apache/pinot/spi/data/Schema.java](https://app.codecov.io/gh/apache/pinot/pull/13905?src=pr&el=tree&filepath=pinot-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fspi%2Fdata%2FSchema.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9TY2hlbWEuamF2YQ==) | 0.00% | [19 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13905?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ot/segment/spi/creator/SegmentGeneratorConfig.java](https://app.codecov.io/gh/apache/pinot/pull/13905?src=pr&el=tree&filepath=pinot-segment-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Fspi%2Fcreator%2FSegmentGeneratorConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2NyZWF0b3IvU2VnbWVudEdlbmVyYXRvckNvbmZpZy5qYXZh) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13905?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...egment/spi/index/metadata/SegmentMetadataImpl.java](https://app.codecov.io/gh/apache/pinot/pull/13905?src=pr&el=tree&filepath=pinot-segment-spi%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Fspi%2Findex%2Fmetadata%2FSegmentMetadataImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1zcGkvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3NlZ21lbnQvc3BpL2luZGV4L21ldGFkYXRhL1NlZ21lbnRNZXRhZGF0YUltcGwuamF2YQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13905?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 (59551e4) and HEAD (f0582ee). Click for more details.
>
> <details><summary>HEAD has 48 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (f0582ee) |
>|------|------|------|
>|integration|7|2|
>|integration2|3|2|
>|temurin|12|2|
>|java-21|7|2|
>|skip-bytebuffers-true|3|1|
>|skip-bytebuffers-false|7|1|
>|unittests|5|0|
>|unittests1|2|0|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #13905 +/- ##
=============================================
- Coverage 61.75% 0.00% -61.76%
=============================================
Files 2436 2505 +69
Lines 133233 138858 +5625
Branches 20636 21571 +935
=============================================
- Hits 82274 0 -82274
- Misses 44911 138858 +93947
+ Partials 6048 0 -6048
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/13905/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/13905/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/13905/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-0.01%)` | :arrow_down: |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/13905/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/13905/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/13905/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/13905/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.63%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/13905/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.75%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/13905/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-27.73%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/13905/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (-61.76%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/13905/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/13905/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/13905/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/13905?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).
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1737782739
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -633,6 +647,55 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
}
}
+ /**
+ * In order to persist complex field metadata, we need to recursively add child field specs
+ * So, each complex field spec will have a property for its child field names and each child field will have its
+ * own properties of the detailed field spec.
+ * E.g. a COMPLEX type `intMap` of Map<String, Integer> has 2 child fields:
+ * - key in STRING type and value in INT type.
+ * Then we will have the following properties to define a COMPLEX field:
+ * column.intMap.childFieldNames = [key, value]
+ * column.intMap$$key.columnType = DIMENSION
+ * column.intMap$$key.dataType = STRING
+ * column.intMap$$key.isSingleValued = true
+ * column.intMap$$value.columnType = DIMENSION
+ * column.intMap$$value.dataType = INT
+ * column.intMap$$value.isSingleValued = true
+ */
+ public static void addFieldSpec(PropertiesConfiguration properties, String column, FieldSpec fieldSpec) {
+ properties.setProperty(getKeyFor(column, COLUMN_TYPE), String.valueOf(fieldSpec.getFieldType()));
+ if (!column.equals(fieldSpec.getName())) {
+ properties.setProperty(getKeyFor(column, COLUMN_NAME), String.valueOf(fieldSpec.getName()));
Review Comment:
Yes for the fieldSpec we could configure a String DimensionSpec called `key`, then during the persistent, the column name is `stringMap$$key`.
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #13905:
URL: https://github.com/apache/pinot/pull/13905
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1735203149
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/ComplexFieldSpec.java:
##########
@@ -87,6 +98,58 @@ public FieldType getFieldType() {
@Override
public String toString() {
- return "field type: COMPLEX, field name: " + _name + ", root data type: " + _dataType;
+ return "field type: COMPLEX, field name: " + _name + ", root data type: " + _dataType + ", child field specs: "
+ + _childFieldSpecs;
+ }
+
+ public static class MapFieldSpec {
+ private final String _fieldName;
+ private final FieldSpec _keyFieldSpec;
+ private final FieldSpec _valueFieldSpec;
+
+ private MapFieldSpec(ComplexFieldSpec complexFieldSpec) {
+ Preconditions.checkState(complexFieldSpec.getChildFieldSpecs().containsKey(KEY_FIELD),
+ "Missing 'key' in the 'childFieldSpec'");
+ Preconditions.checkState(complexFieldSpec.getChildFieldSpecs().containsKey(VALUE_FIELD),
+ "Missing 'value' in the 'childFieldSpec'");
+ _keyFieldSpec = complexFieldSpec.getChildFieldSpec(KEY_FIELD);
+ _valueFieldSpec = complexFieldSpec.getChildFieldSpec(VALUE_FIELD);
+ _fieldName = complexFieldSpec.getName();
+ }
+
+ public String getFieldName() {
+ return _fieldName;
+ }
+
+ public FieldSpec getKeyFieldSpec() {
+ return _keyFieldSpec;
+ }
+
+ public FieldSpec getValueFieldSpec() {
+ return _valueFieldSpec;
+ }
+ }
+
+ public static MapFieldSpec fromMapFieldSpec(ComplexFieldSpec complexFieldSpec) {
+ return new MapFieldSpec(complexFieldSpec);
+ }
+
+ public static ComplexFieldSpec fromMapFieldSpec(MapFieldSpec mapFieldSpec) {
+ return new ComplexFieldSpec(mapFieldSpec.getFieldName(), DataType.MAP, true,
+ Map.of(KEY_FIELD, mapFieldSpec.getKeyFieldSpec(), VALUE_FIELD, mapFieldSpec.getValueFieldSpec()));
+ }
+
+ public static String getFullChildName(String... columns) {
Review Comment:
Added a few. It's used for column metadata persistence.
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1735174694
##########
pinot-core/src/test/java/org/apache/pinot/core/util/CrcUtilsTest.java:
##########
@@ -38,8 +38,8 @@
public class CrcUtilsTest {
private static final File INDEX_DIR = new File(FileUtils.getTempDirectory(), "CrcUtilsTest");
private static final String AVRO_DATA = "data/test_data-mv.avro";
- private static final long EXPECTED_V1_CRC = 2708456273L;
- private static final long EXPECTED_V3_CRC = 2796149869L;
+ private static final long EXPECTED_V1_CRC = 3087108662L;
Review Comment:
Since we need to add extra child field spec, so I refactor the logic and add method `addFieldSpec(properties, column, fieldSpec);`
However this changes the order of column stats in the `metadata.properties` files got changed.
Do you see this an issue for crc change?
E.g.
before:
```
column.column1.cardinality = 51594
column.column1.totalDocs = 100000
column.column1.dataType = INT
column.column1.bitsPerElement = 16
column.column1.lengthOfEachEntry = 0
column.column1.columnType = DIMENSION
column.column1.isSorted = false
column.column1.hasDictionary = true
column.column1.isSingleValues = true
column.column1.maxNumberOfMultiValues = 0
column.column1.totalNumberOfEntries = 100000
column.column1.isAutoGenerated = false
column.column1.minValue = 15935
column.column1.maxValue = 2147475579
column.column1.defaultNullValue = -2147483648
```
after:
```
column.column1.cardinality = 51594
column.column1.totalDocs = 100000
column.column1.bitsPerElement = 16
column.column1.lengthOfEachEntry = 0
column.column1.isSorted = false
column.column1.hasDictionary = true
column.column1.maxNumberOfMultiValues = 0
column.column1.totalNumberOfEntries = 100000
column.column1.isAutoGenerated = false
column.column1.columnType = DIMENSION
column.column1.dataType = INT
column.column1.isSingleValues = true
column.column1.minValue = 15935
column.column1.maxValue = 2147475579
column.column1.defaultNullValue = -2147483648
```
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1737700349
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -633,6 +647,55 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
}
}
+ /**
+ * In order to persist complex field metadata, we need to recursively add child field specs
+ * So, each complex field spec will have a property for its child field names and each child field will have its
+ * own properties of the detailed field spec.
+ * E.g. a COMPLEX type `intMap` of Map<String, Integer> has 2 child fields:
+ * - key in STRING type and value in INT type.
+ * Then we will have the following properties to define a COMPLEX field:
+ * column.intMap.childFieldNames = [key, value]
+ * column.intMap$$key.columnType = DIMENSION
+ * column.intMap$$key.dataType = STRING
+ * column.intMap$$key.isSingleValued = true
+ * column.intMap$$value.columnType = DIMENSION
+ * column.intMap$$value.dataType = INT
+ * column.intMap$$value.isSingleValued = true
+ */
+ public static void addFieldSpec(PropertiesConfiguration properties, String column, FieldSpec fieldSpec) {
Review Comment:
This is only used to recover the ChildFieldSpec for ComplexFieldSpec.
It won't persist any column metadata.
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1737782739
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -633,6 +647,55 @@ public static void addColumnMetadataInfo(PropertiesConfiguration properties, Str
}
}
+ /**
+ * In order to persist complex field metadata, we need to recursively add child field specs
+ * So, each complex field spec will have a property for its child field names and each child field will have its
+ * own properties of the detailed field spec.
+ * E.g. a COMPLEX type `intMap` of Map<String, Integer> has 2 child fields:
+ * - key in STRING type and value in INT type.
+ * Then we will have the following properties to define a COMPLEX field:
+ * column.intMap.childFieldNames = [key, value]
+ * column.intMap$$key.columnType = DIMENSION
+ * column.intMap$$key.dataType = STRING
+ * column.intMap$$key.isSingleValued = true
+ * column.intMap$$value.columnType = DIMENSION
+ * column.intMap$$value.dataType = INT
+ * column.intMap$$value.isSingleValued = true
+ */
+ public static void addFieldSpec(PropertiesConfiguration properties, String column, FieldSpec fieldSpec) {
+ properties.setProperty(getKeyFor(column, COLUMN_TYPE), String.valueOf(fieldSpec.getFieldType()));
+ if (!column.equals(fieldSpec.getName())) {
+ properties.setProperty(getKeyFor(column, COLUMN_NAME), String.valueOf(fieldSpec.getName()));
Review Comment:
Yes for the childFieldSpec we could configure a String DimensionSpec called `key`, then during the persistent, the column name is `stringMap$$key`.
Btw, this is only used for ColumnMetadataImpl persist serde.
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1735398176
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/creator/impl/SegmentColumnarIndexCreator.java:
##########
@@ -475,6 +477,7 @@ private void writeMetadata()
properties.setProperty(DIMENSIONS, _config.getDimensions());
properties.setProperty(METRICS, _config.getMetrics());
properties.setProperty(DATETIME_COLUMNS, _config.getDateTimeColumnNames());
+ properties.setProperty(COMPLEX_COLUMNS, _config.getComplexColumnNames());
Review Comment:
I feel this is the reason for the crc change. Should we consider not adding this field if there is no complex column?
--
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] Support ComplexFieldSpec in Schema and column metadata [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #13905:
URL: https://github.com/apache/pinot/pull/13905#discussion_r1735177330
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/ComplexFieldSpec.java:
##########
@@ -87,6 +98,58 @@ public FieldType getFieldType() {
@Override
public String toString() {
- return "field type: COMPLEX, field name: " + _name + ", root data type: " + _dataType;
+ return "field type: COMPLEX, field name: " + _name + ", root data type: " + _dataType + ", child field specs: "
+ + _childFieldSpecs;
+ }
+
+ public static class MapFieldSpec {
+ private final String _fieldName;
+ private final FieldSpec _keyFieldSpec;
+ private final FieldSpec _valueFieldSpec;
+
+ private MapFieldSpec(ComplexFieldSpec complexFieldSpec) {
+ Preconditions.checkState(complexFieldSpec.getChildFieldSpecs().containsKey(KEY_FIELD),
+ "Missing 'key' in the 'childFieldSpec'");
+ Preconditions.checkState(complexFieldSpec.getChildFieldSpecs().containsKey(VALUE_FIELD),
+ "Missing 'value' in the 'childFieldSpec'");
+ _keyFieldSpec = complexFieldSpec.getChildFieldSpec(KEY_FIELD);
+ _valueFieldSpec = complexFieldSpec.getChildFieldSpec(VALUE_FIELD);
+ _fieldName = complexFieldSpec.getName();
+ }
+
+ public String getFieldName() {
+ return _fieldName;
+ }
+
+ public FieldSpec getKeyFieldSpec() {
+ return _keyFieldSpec;
+ }
+
+ public FieldSpec getValueFieldSpec() {
+ return _valueFieldSpec;
+ }
+ }
+
+ public static MapFieldSpec fromMapFieldSpec(ComplexFieldSpec complexFieldSpec) {
Review Comment:
done.
--
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