You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "INNOCENT-BOY (via GitHub)" <gi...@apache.org> on 2023/06/28 07:08:13 UTC
[GitHub] [pinot] INNOCENT-BOY opened a new pull request, #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
INNOCENT-BOY opened a new pull request, #10991:
URL: https://github.com/apache/pinot/pull/10991
Even though we enable case-insensitive，we found one potential risk in some occasions. My two colleagues want to add one column to the same table in the same pinot environment. But theirs naming style is different. One person add name callType, other people add CallType, and they all succeed. Because we've already enabled case-insensitive option, so they us all could query from Controller UI and no one found potential risk. But one of client report an incident, they can't query from Trino, it will report Multi Entries error.
In my opinion, after we enabled case insensitive, we should't allow user to add the same lowercase column with existed columns.
Waiting for maintainer or reviewers to review!
--
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] Jackie-Jiang commented on a diff in pull request #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10991:
URL: https://github.com/apache/pinot/pull/10991#discussion_r1253942094
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/SchemaUtils.java:
##########
@@ -95,8 +101,14 @@ public static void validate(Schema schema, List<TableConfig> tableConfigs) {
* 6) Schema validations from {@link Schema#validate}
*/
public static void validate(Schema schema) {
+ validate(schema, false);
+ }
+
+ public static void validate(Schema schema, boolean isIgnoreCase) {
schema.validate();
+ List allColumnName = schema.getColumnNames().stream().map(column -> column.toLowerCase())
+ .collect(Collectors.toList());
Review Comment:
We need to create this List only when `isIgnoreCase` is set to `true`. To make it more efficient, we can simply check while adding columns
```suggestion
if (isIgnoreCase) {
Set<String> lowerCaseColumnNames = new HashSet<>;
for (String column : schema.getColumnNames()) {
Preconditions.checkState(lowerCaseColumnNames.add(column.toLowerCase()), ...);
}
}
List allColumnName = schema.getColumnNames().stream().map(column -> column.toLowerCase())
.collect(Collectors.toList());
```
--
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] walterddr commented on a diff in pull request #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10991:
URL: https://github.com/apache/pinot/pull/10991#discussion_r1246860273
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java:
##########
@@ -359,7 +359,8 @@ private void validateSchemaInternal(Schema schema) {
validateSchemaName(schema.getSchemaName());
try {
List<TableConfig> tableConfigs = _pinotHelixResourceManager.getTableConfigsForSchema(schema.getSchemaName());
- SchemaUtils.validate(schema, tableConfigs);
+ boolean isIgnoreCase = _pinotHelixResourceManager.isIgnoreCase();
+ SchemaUtils.validate(schema, tableConfigs, isIgnoreCase);
Review Comment:
no that's not what I meant.
for example in `PinotQueryResource` it inject ControllerConf which has the case sensitivity config key in it presumably. instead of using the injected helix manager you can directly inject the config which is what we wanted here right?
this way you don't need to modify the helix manager API to expose something that's static and will not change associated with zk
--
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] INNOCENT-BOY commented on a diff in pull request #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #10991:
URL: https://github.com/apache/pinot/pull/10991#discussion_r1246134976
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java:
##########
@@ -359,7 +359,8 @@ private void validateSchemaInternal(Schema schema) {
validateSchemaName(schema.getSchemaName());
try {
List<TableConfig> tableConfigs = _pinotHelixResourceManager.getTableConfigsForSchema(schema.getSchemaName());
- SchemaUtils.validate(schema, tableConfigs);
+ boolean isIgnoreCase = _pinotHelixResourceManager.isIgnoreCase();
+ SchemaUtils.validate(schema, tableConfigs, isIgnoreCase);
Review Comment:
Thanks for you review @walterddr !
I think broker starter has already encapsulated this parameter.
```
boolean caseInsensitive =
_brokerConf.getProperty(Helix.ENABLE_CASE_INSENSITIVE_KEY, Helix.DEFAULT_ENABLE_CASE_INSENSITIVE);
TableCache tableCache = new TableCache(_propertyStore, caseInsensitive);
```
The reason why I extract this keyword from Helix ResouceManger is that PinotHelixResourceManger has extract this parameter from HelixAdmin and save it to Table Cache. So I don't think we need find another way to get this parameter. And PinotSchemaRestletResouce has injected an instance _pinotHelixResourceManager. So i don't that is a big deal.
```
Map<String, String> configs = _helixAdmin.getConfig(helixConfigScope,
Arrays.asList(Helix.ENABLE_CASE_INSENSITIVE_KEY));
boolean caseInsensitive = Boolean.parseBoolean(configs.getOrDefault(Helix.ENABLE_CASE_INSENSITIVE_KEY,
Boolean.toString(Helix.DEFAULT_ENABLE_CASE_INSENSITIVE)));
_tableCache = new TableCache(_propertyStore, caseInsensitive);
```
--
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] walterddr commented on a diff in pull request #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10991:
URL: https://github.com/apache/pinot/pull/10991#discussion_r1248907153
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java:
##########
@@ -359,7 +364,10 @@ private void validateSchemaInternal(Schema schema) {
validateSchemaName(schema.getSchemaName());
try {
List<TableConfig> tableConfigs = _pinotHelixResourceManager.getTableConfigsForSchema(schema.getSchemaName());
- SchemaUtils.validate(schema, tableConfigs);
+ boolean isIgnoreCase =
+ Boolean.parseBoolean(_controllerConf.getProperty(CommonConstants.Helix.ENABLE_CASE_INSENSITIVE_KEY,
Review Comment:
:+1: sounds good. didn't know this config is overwrite-able from zk
--
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] INNOCENT-BOY commented on a diff in pull request #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #10991:
URL: https://github.com/apache/pinot/pull/10991#discussion_r1254156577
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/SchemaUtils.java:
##########
@@ -95,8 +101,14 @@ public static void validate(Schema schema, List<TableConfig> tableConfigs) {
* 6) Schema validations from {@link Schema#validate}
*/
public static void validate(Schema schema) {
+ validate(schema, false);
+ }
+
+ public static void validate(Schema schema, boolean isIgnoreCase) {
schema.validate();
+ List allColumnName = schema.getColumnNames().stream().map(column -> column.toLowerCase())
+ .collect(Collectors.toList());
Review Comment:
LGTM, I will update my pr to make it more efficient.
--
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] walterddr commented on a diff in pull request #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10991:
URL: https://github.com/apache/pinot/pull/10991#discussion_r1246860273
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java:
##########
@@ -359,7 +359,8 @@ private void validateSchemaInternal(Schema schema) {
validateSchemaName(schema.getSchemaName());
try {
List<TableConfig> tableConfigs = _pinotHelixResourceManager.getTableConfigsForSchema(schema.getSchemaName());
- SchemaUtils.validate(schema, tableConfigs);
+ boolean isIgnoreCase = _pinotHelixResourceManager.isIgnoreCase();
+ SchemaUtils.validate(schema, tableConfigs, isIgnoreCase);
Review Comment:
no that's not what I meant.
for example in `PinotQueryResource` it inject ControllerConf which has the case sensitivity config key in it presumably. instead of inject helix manager you can directly inject the config which is what we wanted here right?
--
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] INNOCENT-BOY commented on a diff in pull request #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #10991:
URL: https://github.com/apache/pinot/pull/10991#discussion_r1248895668
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java:
##########
@@ -359,7 +364,10 @@ private void validateSchemaInternal(Schema schema) {
validateSchemaName(schema.getSchemaName());
try {
List<TableConfig> tableConfigs = _pinotHelixResourceManager.getTableConfigsForSchema(schema.getSchemaName());
- SchemaUtils.validate(schema, tableConfigs);
+ boolean isIgnoreCase =
+ Boolean.parseBoolean(_controllerConf.getProperty(CommonConstants.Helix.ENABLE_CASE_INSENSITIVE_KEY,
Review Comment:
Thanks for your review and your suggestion. @Jackie-Jiang
I have changed to ingest directly from TableCache other than controller config. Please take a second look.
--
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] walterddr commented on a diff in pull request #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10991:
URL: https://github.com/apache/pinot/pull/10991#discussion_r1245949367
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java:
##########
@@ -359,7 +359,8 @@ private void validateSchemaInternal(Schema schema) {
validateSchemaName(schema.getSchemaName());
try {
List<TableConfig> tableConfigs = _pinotHelixResourceManager.getTableConfigsForSchema(schema.getSchemaName());
- SchemaUtils.validate(schema, tableConfigs);
+ boolean isIgnoreCase = _pinotHelixResourceManager.isIgnoreCase();
+ SchemaUtils.validate(schema, tableConfigs, isIgnoreCase);
Review Comment:
i wonder if this is even necessary to extract this from helix resource manager. eventually this is a startup time config
```
Boolean.parseBoolean(configs.getOrDefault(Helix.ENABLE_CASE_INSENSITIVE_KEY,
Boolean.toString(Helix.DEFAULT_ENABLE_CASE_INSENSITIVE)));
```
we can simply encapsulate in the broker/controller starter?
--
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] INNOCENT-BOY commented on a diff in pull request #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #10991:
URL: https://github.com/apache/pinot/pull/10991#discussion_r1246134976
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java:
##########
@@ -359,7 +359,8 @@ private void validateSchemaInternal(Schema schema) {
validateSchemaName(schema.getSchemaName());
try {
List<TableConfig> tableConfigs = _pinotHelixResourceManager.getTableConfigsForSchema(schema.getSchemaName());
- SchemaUtils.validate(schema, tableConfigs);
+ boolean isIgnoreCase = _pinotHelixResourceManager.isIgnoreCase();
+ SchemaUtils.validate(schema, tableConfigs, isIgnoreCase);
Review Comment:
Thanks for you review @walterddr !
I think broker starter has already encapsulated this parameter.
```
boolean caseInsensitive =
_brokerConf.getProperty(Helix.ENABLE_CASE_INSENSITIVE_KEY, Helix.DEFAULT_ENABLE_CASE_INSENSITIVE);
TableCache tableCache = new TableCache(_propertyStore, caseInsensitive);
```
The reason why I extract this keyword from Helix ResouceManger is that PinotHelixResourceManger has extract this parameter from HelixAdmin and save it to Table Cache. So I don't think we need find another way to get this parameter. And PinotSchemaRestletResouce has injected an instance _pinotHelixResourceManager. So i don't think that is a big deal.
```
Map<String, String> configs = _helixAdmin.getConfig(helixConfigScope,
Arrays.asList(Helix.ENABLE_CASE_INSENSITIVE_KEY));
boolean caseInsensitive = Boolean.parseBoolean(configs.getOrDefault(Helix.ENABLE_CASE_INSENSITIVE_KEY,
Boolean.toString(Helix.DEFAULT_ENABLE_CASE_INSENSITIVE)));
_tableCache = new TableCache(_propertyStore, caseInsensitive);
```
--
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 #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10991:
URL: https://github.com/apache/pinot/pull/10991#issuecomment-1610931375
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10991?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10991](https://app.codecov.io/gh/apache/pinot/pull/10991?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (d434f54) into [master](https://app.codecov.io/gh/apache/pinot/commit/04bdb9a39743569aaacfcb07e2cc7eaa08854791?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (04bdb9a) will **increase** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10991 +/- ##
=========================================
Coverage 0.11% 0.11%
=========================================
Files 2192 2138 -54
Lines 118080 115588 -2492
Branches 17883 17583 -300
=========================================
Hits 137 137
+ Misses 117923 115431 -2492
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `?` | |
| integration1temurin17 | `?` | |
| integration1temurin20 | `?` | |
| integration2temurin11 | `?` | |
| integration2temurin17 | `?` | |
| integration2temurin20 | `?` | |
| unittests1temurin11 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `0.11% <0.00%> (-0.01%)` | :arrow_down: |
| unittests2temurin17 | `0.11% <0.00%> (-0.01%)` | :arrow_down: |
| unittests2temurin20 | `0.11% <0.00%> (-0.01%)` | :arrow_down: |
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.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10991?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ller/api/resources/PinotSchemaRestletResource.java](https://app.codecov.io/gh/apache/pinot/pull/10991?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90U2NoZW1hUmVzdGxldFJlc291cmNlLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://app.codecov.io/gh/apache/pinot/pull/10991?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [.../apache/pinot/segment/local/utils/SchemaUtils.java](https://app.codecov.io/gh/apache/pinot/pull/10991?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9TY2hlbWFVdGlscy5qYXZh) | `0.00% <0.00%> (ø)` | |
... and [54 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10991/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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=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
[GitHub] [pinot] INNOCENT-BOY commented on a diff in pull request #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #10991:
URL: https://github.com/apache/pinot/pull/10991#discussion_r1246134976
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java:
##########
@@ -359,7 +359,8 @@ private void validateSchemaInternal(Schema schema) {
validateSchemaName(schema.getSchemaName());
try {
List<TableConfig> tableConfigs = _pinotHelixResourceManager.getTableConfigsForSchema(schema.getSchemaName());
- SchemaUtils.validate(schema, tableConfigs);
+ boolean isIgnoreCase = _pinotHelixResourceManager.isIgnoreCase();
+ SchemaUtils.validate(schema, tableConfigs, isIgnoreCase);
Review Comment:
Thanks for you review @walterddr !
I think broker starter has already encapsulated this parameter.
```
boolean caseInsensitive =
_brokerConf.getProperty(Helix.ENABLE_CASE_INSENSITIVE_KEY, Helix.DEFAULT_ENABLE_CASE_INSENSITIVE);
TableCache tableCache = new TableCache(_propertyStore, caseInsensitive);
```
The reason why I extract this keyword from Helix ResouceManger is that PinotHelixResourceManger has extract this parameter from HelixAdmin and save it to Table Cache. So I don't think we need find another way to get this parameter. And PinotSchemaRestletResouce has and auto-injected instance _pinotHelixResourceManager. So i don't think that is a big deal.
```
Map<String, String> configs = _helixAdmin.getConfig(helixConfigScope,
Arrays.asList(Helix.ENABLE_CASE_INSENSITIVE_KEY));
boolean caseInsensitive = Boolean.parseBoolean(configs.getOrDefault(Helix.ENABLE_CASE_INSENSITIVE_KEY,
Boolean.toString(Helix.DEFAULT_ENABLE_CASE_INSENSITIVE)));
_tableCache = new TableCache(_propertyStore, caseInsensitive);
```
--
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] INNOCENT-BOY commented on a diff in pull request #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #10991:
URL: https://github.com/apache/pinot/pull/10991#discussion_r1254156577
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/SchemaUtils.java:
##########
@@ -95,8 +101,14 @@ public static void validate(Schema schema, List<TableConfig> tableConfigs) {
* 6) Schema validations from {@link Schema#validate}
*/
public static void validate(Schema schema) {
+ validate(schema, false);
+ }
+
+ public static void validate(Schema schema, boolean isIgnoreCase) {
schema.validate();
+ List allColumnName = schema.getColumnNames().stream().map(column -> column.toLowerCase())
+ .collect(Collectors.toList());
Review Comment:
Great Suggestion! I have updated my pr to make it more efficient. @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
[GitHub] [pinot] Jackie-Jiang merged pull request #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #10991:
URL: https://github.com/apache/pinot/pull/10991
--
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] walterddr commented on a diff in pull request #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10991:
URL: https://github.com/apache/pinot/pull/10991#discussion_r1248907153
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java:
##########
@@ -359,7 +364,10 @@ private void validateSchemaInternal(Schema schema) {
validateSchemaName(schema.getSchemaName());
try {
List<TableConfig> tableConfigs = _pinotHelixResourceManager.getTableConfigsForSchema(schema.getSchemaName());
- SchemaUtils.validate(schema, tableConfigs);
+ boolean isIgnoreCase =
+ Boolean.parseBoolean(_controllerConf.getProperty(CommonConstants.Helix.ENABLE_CASE_INSENSITIVE_KEY,
Review Comment:
:+1: sounds
--
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] Jackie-Jiang commented on a diff in pull request #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #10991:
URL: https://github.com/apache/pinot/pull/10991#discussion_r1248349644
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java:
##########
@@ -359,7 +364,10 @@ private void validateSchemaInternal(Schema schema) {
validateSchemaName(schema.getSchemaName());
try {
List<TableConfig> tableConfigs = _pinotHelixResourceManager.getTableConfigsForSchema(schema.getSchemaName());
- SchemaUtils.validate(schema, tableConfigs);
+ boolean isIgnoreCase =
+ Boolean.parseBoolean(_controllerConf.getProperty(CommonConstants.Helix.ENABLE_CASE_INSENSITIVE_KEY,
Review Comment:
I think this config is actually read from the ZK (cluster config) instead of controller config. Please take a look at `PinotHelixResourceManager.start()`.
We can probably directly inject the `TableCache` or case-insensitive boolean
--
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] INNOCENT-BOY commented on a diff in pull request #10991: [enhance]: When enable case-insensitive, don't allow to add newly col…
Posted by "INNOCENT-BOY (via GitHub)" <gi...@apache.org>.
INNOCENT-BOY commented on code in PR #10991:
URL: https://github.com/apache/pinot/pull/10991#discussion_r1247696908
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java:
##########
@@ -359,7 +359,8 @@ private void validateSchemaInternal(Schema schema) {
validateSchemaName(schema.getSchemaName());
try {
List<TableConfig> tableConfigs = _pinotHelixResourceManager.getTableConfigsForSchema(schema.getSchemaName());
- SchemaUtils.validate(schema, tableConfigs);
+ boolean isIgnoreCase = _pinotHelixResourceManager.isIgnoreCase();
+ SchemaUtils.validate(schema, tableConfigs, isIgnoreCase);
Review Comment:
Hi @walterddr , I got that you mean and I have accepted your advice. Do you have any other advices?
--
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