You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "shounakmk219 (via GitHub)" <gi...@apache.org> on 2023/04/20 18:11:02 UTC
[GitHub] [pinot] shounakmk219 opened a new pull request, #10655: API to expose the contract/rules imposed by pinot on tableConfig
shounakmk219 opened a new pull request, #10655:
URL: https://github.com/apache/pinot/pull/10655
This PR tries to address the items mention in [issue #10647](https://github.com/apache/pinot/issues/10647)
Changes :
1. Moved the switch case logic from `Schema.validate()` to `Schema.validate(FieldType fieldType, DataType dataType)` so that the code can be reused to just validate if a DataType is allowed by a FieldType.
2. Created `FieldSpec.FIELD_SPEC_METADATA` to hold the spec metadata.
3. Created a wrapper to hold the spec metadata. Currently is just has the valid (fieldType, dataType) pairs along with the respective default null values.
```
{
"fieldTypes": {
"METRIC": {
"allowedDataTypes": [
{
"name": "INT",
"nullDefault": 0
},
{
"name": "STRING",
"nullDefault": "null"
},
.
.
.
]
},
"DIMENSION": {
"allowedDataTypes": [
{
"name": "INT",
"nullDefault": -2147483648
},
{
"name": "STRING",
"nullDefault": "null"
},
.
.
.
]
},
.
.
.
}
}
```
4. Expose the metadata from a new API endpoint at `/tableConfigs/specMetadata`
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #10655: API to expose the contract/rules imposed by pinot on tableConfig
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10655:
URL: https://github.com/apache/pinot/pull/10655#issuecomment-1516813309
## [Codecov](https://codecov.io/gh/apache/pinot/pull/10655?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10655](https://codecov.io/gh/apache/pinot/pull/10655?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9209b69) into [master](https://codecov.io/gh/apache/pinot/commit/c7e05a7b58f5435080ab26e9ef8888e2b07dd974?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c7e05a7) will **increase** coverage by `40.82%`.
> The diff coverage is `88.37%`.
```diff
@@ Coverage Diff @@
## master #10655 +/- ##
=============================================
+ Coverage 27.66% 68.48% +40.82%
- Complexity 58 6487 +6429
=============================================
Files 2090 2108 +18
Lines 112578 113568 +990
Branches 16972 17118 +146
=============================================
+ Hits 31145 77782 +46637
+ Misses 78333 30251 -48082
- Partials 3100 5535 +2435
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `24.36% <0.00%> (-0.11%)` | :arrow_down: |
| integration2 | `?` | |
| unittests1 | `67.90% <95.00%> (?)` | |
| unittests2 | `13.88% <0.00%> (?)` | |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10655?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ler/api/resources/TableConfigsRestletResource.java](https://codecov.io/gh/apache/pinot/pull/10655?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1RhYmxlQ29uZmlnc1Jlc3RsZXRSZXNvdXJjZS5qYXZh) | `78.07% <0.00%> (+55.24%)` | :arrow_up: |
| [...rc/main/java/org/apache/pinot/spi/data/Schema.java](https://codecov.io/gh/apache/pinot/pull/10655?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9TY2hlbWEuamF2YQ==) | `75.21% <86.66%> (+75.21%)` | :arrow_up: |
| [...main/java/org/apache/pinot/spi/data/FieldSpec.java](https://codecov.io/gh/apache/pinot/pull/10655?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9GaWVsZFNwZWMuamF2YQ==) | `84.87% <100.00%> (+84.87%)` | :arrow_up: |
... and [1571 files with indirect coverage changes](https://codecov.io/gh/apache/pinot/pull/10655/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [pinot] saurabhd336 commented on a diff in pull request #10655: API to expose the contract/rules imposed by pinot on tableConfig
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on code in PR #10655:
URL: https://github.com/apache/pinot/pull/10655#discussion_r1173615900
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/TableConfigsRestletResource.java:
##########
@@ -126,6 +127,22 @@ public String listConfigs() {
}
}
+ /**
+ * Gets the metadata on the valid {@link org.apache.pinot.spi.data.FieldSpec.DataType} for each
+ * {@link org.apache.pinot.spi.data.FieldSpec.FieldType} and the default null values for each combination
+ */
+ @GET
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("/tableConfigs/specMetadata")
+ @ApiOperation(value = "Get TableConfig metadata", notes = "Get TableConfig metadata")
+ public String getFieldSpecMetadata() {
+ try {
+ return JsonUtils.objectToString(FieldSpec.FIELD_SPEC_METADATA);
Review Comment:
Return just the object. It'll automatically be jsonified.
--
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
[GitHub] [pinot] saurabhd336 merged pull request #10655: API to expose the contract/rules imposed by pinot on tableConfig
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 merged PR #10655:
URL: https://github.com/apache/pinot/pull/10655
--
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
[GitHub] [pinot] saurabhd336 commented on a diff in pull request #10655: API to expose the contract/rules imposed by pinot on tableConfig
Posted by "saurabhd336 (via GitHub)" <gi...@apache.org>.
saurabhd336 commented on code in PR #10655:
URL: https://github.com/apache/pinot/pull/10655#discussion_r1173620687
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -563,4 +592,30 @@ public boolean isBackwardCompatibleWith(FieldSpec oldFieldSpec) {
&& EqualityUtils.isEqual(_dataType, oldFieldSpec._dataType)
&& EqualityUtils.isEqual(_isSingleValueField, oldFieldSpec._isSingleValueField);
}
+
+ public static class FieldSpecMetadata {
+ public Map<FieldType, FieldTypeMetadata> _fieldTypes = new HashMap<>();
+
+ void put(FieldType type, FieldTypeMetadata metadata) {
+ _fieldTypes.put(type, metadata);
+ }
+ }
+
+ public static class FieldTypeMetadata {
+ public List<DataTypeMetadata> _allowedDataTypes = new ArrayList<>();
+
+ void add(DataTypeMetadata metadata) {
+ _allowedDataTypes.add(metadata);
+ }
+ }
+
+ public static class DataTypeMetadata {
+ public FieldSpec.DataType _name;
Review Comment:
DataType has a bunch of other useful properties that can be part of the response.
```
private final DataType _storedType;
private final int _size;
private final boolean _sortable;
private final boolean _numeric;
Think we should include all that in the API result.
--
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
[GitHub] [pinot] shounakmk219 commented on a diff in pull request #10655: API to expose the contract/rules imposed by pinot on tableConfig
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #10655:
URL: https://github.com/apache/pinot/pull/10655#discussion_r1173636417
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -563,4 +592,30 @@ public boolean isBackwardCompatibleWith(FieldSpec oldFieldSpec) {
&& EqualityUtils.isEqual(_dataType, oldFieldSpec._dataType)
&& EqualityUtils.isEqual(_isSingleValueField, oldFieldSpec._isSingleValueField);
}
+
+ public static class FieldSpecMetadata {
+ public Map<FieldType, FieldTypeMetadata> _fieldTypes = new HashMap<>();
+
+ void put(FieldType type, FieldTypeMetadata metadata) {
+ _fieldTypes.put(type, metadata);
+ }
+ }
+
+ public static class FieldTypeMetadata {
+ public List<DataTypeMetadata> _allowedDataTypes = new ArrayList<>();
+
+ void add(DataTypeMetadata metadata) {
+ _allowedDataTypes.add(metadata);
+ }
+ }
+
+ public static class DataTypeMetadata {
+ public FieldSpec.DataType _name;
Review Comment:
That's a good point. Maybe I need to introduce a separate `dataTypes` property at `fieldTypes` level so as to not have the `dataType` specific info under `allowedDataTypes ` and that too with redundant replication.
--
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
[GitHub] [pinot] mcvsubbu commented on pull request #10655: API to expose the contract/rules imposed by pinot on tableConfig
Posted by "mcvsubbu (via GitHub)" <gi...@apache.org>.
mcvsubbu commented on PR #10655:
URL: https://github.com/apache/pinot/pull/10655#issuecomment-1518320254
I understand the re-org of code to have metadata on fieldSpec, but why expose this via API? How will a user of Pinot make use of this API?
Should we call the API `/schemata/fieldSpec`? Is this endpoint always going to be some general stuff (i.e. tableName, segmentName, etc. are not an argument)?
--
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
[GitHub] [pinot] shounakmk219 commented on pull request #10655: API to expose the contract/rules imposed by pinot on tableConfig
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on PR #10655:
URL: https://github.com/apache/pinot/pull/10655#issuecomment-1526993617
Hey @mcvsubbu I have changed the endpoint path as suggested, let me know if the above comment addressed your concerns.
--
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
[GitHub] [pinot] shounakmk219 commented on a diff in pull request #10655: API to expose the contract/rules imposed by pinot on tableConfig
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on code in PR #10655:
URL: https://github.com/apache/pinot/pull/10655#discussion_r1179005262
##########
pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java:
##########
@@ -563,4 +592,30 @@ public boolean isBackwardCompatibleWith(FieldSpec oldFieldSpec) {
&& EqualityUtils.isEqual(_dataType, oldFieldSpec._dataType)
&& EqualityUtils.isEqual(_isSingleValueField, oldFieldSpec._isSingleValueField);
}
+
+ public static class FieldSpecMetadata {
+ public Map<FieldType, FieldTypeMetadata> _fieldTypes = new HashMap<>();
+
+ void put(FieldType type, FieldTypeMetadata metadata) {
+ _fieldTypes.put(type, metadata);
+ }
+ }
+
+ public static class FieldTypeMetadata {
+ public List<DataTypeMetadata> _allowedDataTypes = new ArrayList<>();
+
+ void add(DataTypeMetadata metadata) {
+ _allowedDataTypes.add(metadata);
+ }
+ }
+
+ public static class DataTypeMetadata {
+ public FieldSpec.DataType _name;
Review Comment:
Updated the wrapper structure to accommodate the data type properties.
--
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
[GitHub] [pinot] shounakmk219 commented on pull request #10655: API to expose the contract/rules imposed by pinot on tableConfig
Posted by "shounakmk219 (via GitHub)" <gi...@apache.org>.
shounakmk219 commented on PR #10655:
URL: https://github.com/apache/pinot/pull/10655#issuecomment-1519082989
Hey @mcvsubbu as mentioned in the issue (link in description) the API consumer will be mostly another application built on top of pinot rather than a pinot user. Motivation is to enable client side user data input validations.
The endpoint should return in general what are the constraints imposed by pinot on the table config or schema. It's not specific to any particular table or segment. Using this info client application should be able to impose data validations dynamically.
I have no strong reason behind the endpoint path, I can change it to `/schemas/fieldSpec`
--
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