[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792:
URL: https://github.com/apache/calcite/pull/1792#discussion_r663638994
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -67,33 +69,72 @@ public CalciteSchema add(String name, Schema schema) {
return calciteSchema;
}
+ private String caseInsensitiveLookup(Set candidates, String name) {
+// Exact string lookup
+if (candidates.contains(name)) {
+ return name;
+}
+// Upper case string lookup
+final String upperCaseName = name.toUpperCase(Locale.ROOT);
+if (candidates.contains(upperCaseName)) {
+ return upperCaseName;
+}
+// Lower case string lookup
+final String lowerCaseName = name.toLowerCase(Locale.ROOT);
+if (candidates.contains(lowerCaseName)) {
+ return lowerCaseName;
+}
+// Fall through: Set iteration
+for (String candidate: candidates) {
+ if (candidate.equalsIgnoreCase(name)) {
+return candidate;
+ }
+}
+return null;
+ }
+
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final String schemaName2 = caseSensitive ? schemaName :
caseInsensitiveLookup(
+schema.getSubSchemaNames(), schemaName);
+if (schemaName2 == null) {
+ return null;
}
-return null;
+final Schema s = schema.getSubSchema(schemaName2);
+if (s == null) {
+ return null;
+}
+return new SimpleCalciteSchema(this, s, schemaName2);
}
protected TableEntry getImplicitTable(String tableName,
boolean caseSensitive) {
// Check implicit tables.
-Table table = schema.getTable(tableName);
-if (table != null) {
- return tableEntry(tableName, table);
+final String tableName2 = caseSensitive ? tableName :
caseInsensitiveLookup(
+schema.getTableNames(), tableName);
+if (tableName2 == null) {
Review comment:
> @DonnyZone Moving discussion here. why open a new PR? what's wrong
with this one?
There are conflicts now. I will take time to rebase it.
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r381769788
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -67,33 +69,72 @@ public CalciteSchema add(String name, Schema schema) {
return calciteSchema;
}
+ private String caseInsensitiveLookup(Set candidates, String name) {
+// Exact string lookup
+if (candidates.contains(name)) {
+ return name;
+}
+// Upper case string lookup
+final String upperCaseName = name.toUpperCase(Locale.ROOT);
+if (candidates.contains(upperCaseName)) {
+ return upperCaseName;
+}
+// Lower case string lookup
+final String lowerCaseName = name.toLowerCase(Locale.ROOT);
+if (candidates.contains(lowerCaseName)) {
+ return lowerCaseName;
+}
+// Fall through: Set iteration
+for (String candidate: candidates) {
+ if (candidate.equalsIgnoreCase(name)) {
+return candidate;
+ }
+}
+return null;
+ }
+
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final String schemaName2 = caseSensitive ? schemaName :
caseInsensitiveLookup(
+schema.getSubSchemaNames(), schemaName);
+if (schemaName2 == null) {
+ return null;
}
-return null;
+final Schema s = schema.getSubSchema(schemaName2);
+if (s == null) {
+ return null;
+}
+return new SimpleCalciteSchema(this, s, schemaName2);
}
protected TableEntry getImplicitTable(String tableName,
boolean caseSensitive) {
// Check implicit tables.
-Table table = schema.getTable(tableName);
-if (table != null) {
- return tableEntry(tableName, table);
+final String tableName2 = caseSensitive ? tableName :
caseInsensitiveLookup(
+schema.getTableNames(), tableName);
+if (tableName2 == null) {
Review comment:
> can we just keep it as it is now and do not support the case-sensitivity
look-up
Case-insensitive? Current `SimpleCalciteSchema` supports only
case-sensitive lookup.
> keep the SimpleCalciteSchema as simple as possible
Do you mean the simple code style? In logic, this PR adds a branch for
case-insensitive, bringing no burden to case-sensitive branch.
In our production environment, we use the default `CachingCalciteSchema`.
Therefore, I do not have strong requirement for this feature. The only worry is
that users may get incorrect result when using `SimpleCalciteSchema` for
case-insensitive lookup.
Since this Jira issue is filed by @zabetak, how about your opinion?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r381769788
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -67,33 +69,72 @@ public CalciteSchema add(String name, Schema schema) {
return calciteSchema;
}
+ private String caseInsensitiveLookup(Set candidates, String name) {
+// Exact string lookup
+if (candidates.contains(name)) {
+ return name;
+}
+// Upper case string lookup
+final String upperCaseName = name.toUpperCase(Locale.ROOT);
+if (candidates.contains(upperCaseName)) {
+ return upperCaseName;
+}
+// Lower case string lookup
+final String lowerCaseName = name.toLowerCase(Locale.ROOT);
+if (candidates.contains(lowerCaseName)) {
+ return lowerCaseName;
+}
+// Fall through: Set iteration
+for (String candidate: candidates) {
+ if (candidate.equalsIgnoreCase(name)) {
+return candidate;
+ }
+}
+return null;
+ }
+
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final String schemaName2 = caseSensitive ? schemaName :
caseInsensitiveLookup(
+schema.getSubSchemaNames(), schemaName);
+if (schemaName2 == null) {
+ return null;
}
-return null;
+final Schema s = schema.getSubSchema(schemaName2);
+if (s == null) {
+ return null;
+}
+return new SimpleCalciteSchema(this, s, schemaName2);
}
protected TableEntry getImplicitTable(String tableName,
boolean caseSensitive) {
// Check implicit tables.
-Table table = schema.getTable(tableName);
-if (table != null) {
- return tableEntry(tableName, table);
+final String tableName2 = caseSensitive ? tableName :
caseInsensitiveLookup(
+schema.getTableNames(), tableName);
+if (tableName2 == null) {
Review comment:
> can we just keep it as it is now and do not support the case-sensitivity
look-up
Case-insensitive? Current SimpleCalciteSchema supports only case-sensitive
lookup.
> keep the SimpleCalciteSchema as simple as possible
Do you mean the simple code style? In logic, this PR adds a branch for
case-insensitive, bringing no burden to case-sensitive branch.
In our production environment, we use the default `CachingCalciteSchema`.
Therefore, I do not have strong requirement for this feature. The only worry is
that users may get incorrect result when using `SimpleCalciteSchema` for
case-insensitive lookup.
Since this Jira issue is filed by @zabetak, how about your opinion?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r381093307
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -67,33 +69,72 @@ public CalciteSchema add(String name, Schema schema) {
return calciteSchema;
}
+ private String caseInsensitiveLookup(Set candidates, String name) {
+// Exact string lookup
+if (candidates.contains(name)) {
+ return name;
+}
+// Upper case string lookup
+final String upperCaseName = name.toUpperCase(Locale.ROOT);
+if (candidates.contains(upperCaseName)) {
+ return upperCaseName;
+}
+// Lower case string lookup
+final String lowerCaseName = name.toLowerCase(Locale.ROOT);
+if (candidates.contains(lowerCaseName)) {
+ return lowerCaseName;
+}
+// Fall through: Set iteration
+for (String candidate: candidates) {
+ if (candidate.equalsIgnoreCase(name)) {
+return candidate;
+ }
+}
+return null;
+ }
+
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final String schemaName2 = caseSensitive ? schemaName :
caseInsensitiveLookup(
+schema.getSubSchemaNames(), schemaName);
+if (schemaName2 == null) {
+ return null;
}
-return null;
+final Schema s = schema.getSubSchema(schemaName2);
+if (s == null) {
+ return null;
+}
+return new SimpleCalciteSchema(this, s, schemaName2);
}
protected TableEntry getImplicitTable(String tableName,
boolean caseSensitive) {
// Check implicit tables.
-Table table = schema.getTable(tableName);
-if (table != null) {
- return tableEntry(tableName, table);
+final String tableName2 = caseSensitive ? tableName :
caseInsensitiveLookup(
+schema.getTableNames(), tableName);
+if (tableName2 == null) {
Review comment:
That may be same as `CachingCalciteSchema`. I guess if we bookeep or
maintain something (e.g., `CaseSensitiveKey`, `CaseInSensitiveKey`), the
structure will be optimized and evolved to the efficient `NameSet`, as that in
`CachingCalciteSchema` (i.e., `Cache`).
`CachingCalciteSchema` is used by default in Calcite. According to their
names, `SimpleCalciteSchema` may be different, and query items (i.e.,
schema/table/type) on the fly?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r381093307
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -67,33 +69,72 @@ public CalciteSchema add(String name, Schema schema) {
return calciteSchema;
}
+ private String caseInsensitiveLookup(Set candidates, String name) {
+// Exact string lookup
+if (candidates.contains(name)) {
+ return name;
+}
+// Upper case string lookup
+final String upperCaseName = name.toUpperCase(Locale.ROOT);
+if (candidates.contains(upperCaseName)) {
+ return upperCaseName;
+}
+// Lower case string lookup
+final String lowerCaseName = name.toLowerCase(Locale.ROOT);
+if (candidates.contains(lowerCaseName)) {
+ return lowerCaseName;
+}
+// Fall through: Set iteration
+for (String candidate: candidates) {
+ if (candidate.equalsIgnoreCase(name)) {
+return candidate;
+ }
+}
+return null;
+ }
+
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final String schemaName2 = caseSensitive ? schemaName :
caseInsensitiveLookup(
+schema.getSubSchemaNames(), schemaName);
+if (schemaName2 == null) {
+ return null;
}
-return null;
+final Schema s = schema.getSubSchema(schemaName2);
+if (s == null) {
+ return null;
+}
+return new SimpleCalciteSchema(this, s, schemaName2);
}
protected TableEntry getImplicitTable(String tableName,
boolean caseSensitive) {
// Check implicit tables.
-Table table = schema.getTable(tableName);
-if (table != null) {
- return tableEntry(tableName, table);
+final String tableName2 = caseSensitive ? tableName :
caseInsensitiveLookup(
+schema.getTableNames(), tableName);
+if (tableName2 == null) {
Review comment:
That may be same as `CachingCalciteSchema`. I guess if we bookeep or
maintain something (e.g., `CaseSensitiveKey`, `CaseInSensitiveKey`), the
structure will be optimized and evolved to the efficient `NameSet` in the end,
as that in `CachingCalciteSchema` (i.e., `Cache`).
`CachingCalciteSchema` is used by default in Calcite. According to their
names, `SimpleCalciteSchema` may be different, and query items (i.e.,
schema/table/type) on the fly?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r379978508
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -67,33 +69,72 @@ public CalciteSchema add(String name, Schema schema) {
return calciteSchema;
}
+ private String caseInsensitiveLookup(Set candidates, String name) {
+// Exact string lookup
+if (candidates.contains(name)) {
+ return name;
+}
+// Upper case string lookup
+final String upperCaseName = name.toUpperCase(Locale.ROOT);
+if (candidates.contains(upperCaseName)) {
+ return upperCaseName;
+}
+// Lower case string lookup
+final String lowerCaseName = name.toLowerCase(Locale.ROOT);
+if (candidates.contains(lowerCaseName)) {
+ return lowerCaseName;
+}
+// Fall through: Set iteration
+for (String candidate: candidates) {
+ if (candidate.equalsIgnoreCase(name)) {
+return candidate;
+ }
+}
+return null;
+ }
+
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final String schemaName2 = caseSensitive ? schemaName :
caseInsensitiveLookup(
+schema.getSubSchemaNames(), schemaName);
+if (schemaName2 == null) {
+ return null;
}
-return null;
+final Schema s = schema.getSubSchema(schemaName2);
+if (s == null) {
+ return null;
+}
+return new SimpleCalciteSchema(this, s, schemaName2);
}
protected TableEntry getImplicitTable(String tableName,
boolean caseSensitive) {
// Check implicit tables.
-Table table = schema.getTable(tableName);
-if (table != null) {
- return tableEntry(tableName, table);
+final String tableName2 = caseSensitive ? tableName :
caseInsensitiveLookup(
+schema.getTableNames(), tableName);
+if (tableName2 == null) {
Review comment:
I ever thought about abstracting an interface. But it may bring complexity.
(1) Different with name matcher, case (in)sensitive lookup is not widely
used. Even for `CalciteSchema`, there is already a more general and efficient
interface `NamedMap`. This piece of code is just for `SimpleCalciteSchema`,
without maintaining cache.
(2) For performance, we should prevent to call
`getTableNames`/`getSubSchemas`/`getTypeNames` when case sensitive. Therefore,
if we abstract the logic, the interface arguments need to contain functions. It
may be too complex.
Do you have any suggestions?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r378244829
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -67,31 +69,67 @@ public CalciteSchema add(String name, Schema schema) {
return calciteSchema;
}
+ private String caseInsensitiveLookup(Set candidates, String name) {
+// Exact string lookup
+if (candidates.contains(name)) {
+ return name;
+}
+// Upper case string lookup
+final String upperCaseName = name.toUpperCase(Locale.ROOT);
+if (candidates.contains(upperCaseName)) {
+ return upperCaseName;
+}
+// Lower case string lookup
+final String lowerCaseName = name.toLowerCase(Locale.ROOT);
+if (candidates.contains(lowerCaseName)) {
+ return lowerCaseName;
+}
+// Fall through: Set iteration
+for (String candidate: candidates) {
+ if (candidate.equalsIgnoreCase(name)) {
+return candidate;
+ }
+}
+return null;
+ }
+
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final String schemaName2 = caseSensitive ? schemaName :
caseInsensitiveLookup(
+schema.getSubSchemaNames(), schemaName);
+if (schemaName2 != null) {
+ final Schema s = schema.getSubSchema(schemaName2);
Review comment:
It seems to be difficult to achieve both efficiency and elegant stlye. Maybe
some functional languages (e.g., Scala) provide sugar :).
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r378236745
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -67,31 +69,67 @@ public CalciteSchema add(String name, Schema schema) {
return calciteSchema;
}
+ private String caseInsensitiveLookup(Set candidates, String name) {
+// Exact string lookup
+if (candidates.contains(name)) {
+ return name;
+}
+// Upper case string lookup
+final String upperCaseName = name.toUpperCase(Locale.ROOT);
+if (candidates.contains(upperCaseName)) {
+ return upperCaseName;
+}
+// Lower case string lookup
+final String lowerCaseName = name.toLowerCase(Locale.ROOT);
+if (candidates.contains(lowerCaseName)) {
+ return lowerCaseName;
+}
+// Fall through: Set iteration
+for (String candidate: candidates) {
+ if (candidate.equalsIgnoreCase(name)) {
+return candidate;
+ }
+}
+return null;
+ }
+
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final String schemaName2 = caseSensitive ? schemaName :
caseInsensitiveLookup(
+schema.getSubSchemaNames(), schemaName);
+if (schemaName2 != null) {
+ final Schema s = schema.getSubSchema(schemaName2);
Review comment:
Ok, thanks for clarification.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r378229570
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -67,31 +69,67 @@ public CalciteSchema add(String name, Schema schema) {
return calciteSchema;
}
+ private String caseInsensitiveLookup(Set candidates, String name) {
+// Exact string lookup
+if (candidates.contains(name)) {
+ return name;
+}
+// Upper case string lookup
+final String upperCaseName = name.toUpperCase(Locale.ROOT);
+if (candidates.contains(upperCaseName)) {
+ return upperCaseName;
+}
+// Lower case string lookup
+final String lowerCaseName = name.toLowerCase(Locale.ROOT);
+if (candidates.contains(lowerCaseName)) {
+ return lowerCaseName;
+}
+// Fall through: Set iteration
+for (String candidate: candidates) {
+ if (candidate.equalsIgnoreCase(name)) {
+return candidate;
+ }
+}
+return null;
+ }
+
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final String schemaName2 = caseSensitive ? schemaName :
caseInsensitiveLookup(
+schema.getSubSchemaNames(), schemaName);
+if (schemaName2 != null) {
+ final Schema s = schema.getSubSchema(schemaName2);
Review comment:
Is the following code style more efficient than current implementation? If
`xxx==null`, it also returns `null` fast willout doing extra work.
```
if (schemaName2 == null) {
return null;
}
final Schema s = schema.getSubSchema(schemaName2);
if (s == null) {
return null;
}
return new SimpleCalciteSchema(this, s, schemaName2);
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r378229570
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -67,31 +69,67 @@ public CalciteSchema add(String name, Schema schema) {
return calciteSchema;
}
+ private String caseInsensitiveLookup(Set candidates, String name) {
+// Exact string lookup
+if (candidates.contains(name)) {
+ return name;
+}
+// Upper case string lookup
+final String upperCaseName = name.toUpperCase(Locale.ROOT);
+if (candidates.contains(upperCaseName)) {
+ return upperCaseName;
+}
+// Lower case string lookup
+final String lowerCaseName = name.toLowerCase(Locale.ROOT);
+if (candidates.contains(lowerCaseName)) {
+ return lowerCaseName;
+}
+// Fall through: Set iteration
+for (String candidate: candidates) {
+ if (candidate.equalsIgnoreCase(name)) {
+return candidate;
+ }
+}
+return null;
+ }
+
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final String schemaName2 = caseSensitive ? schemaName :
caseInsensitiveLookup(
+schema.getSubSchemaNames(), schemaName);
+if (schemaName2 != null) {
+ final Schema s = schema.getSubSchema(schemaName2);
Review comment:
Is the following code style more efficient?
```
if (schemaName2 == null) {
return null;
}
final Schema s = schema.getSubSchema(schemaName2);
if (s == null) {
return null;
}
return new SimpleCalciteSchema(this, s, schemaName2);
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r378211336
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -67,31 +69,79 @@ public CalciteSchema add(String name, Schema schema) {
return calciteSchema;
}
+ private String caseInsensitiveLookup(Set candidates, String name) {
+// Exact string lookup
+if (candidates.contains(name)) {
+ return name;
+}
+// Upper case string lookup
+final String upperCaseName = name.toUpperCase(Locale.ROOT);
+if (candidates.contains(upperCaseName)) {
+ return upperCaseName;
+}
+// Lower case string lookup
+final String lowerCaseName = name.toLowerCase(Locale.ROOT);
+if (candidates.contains(lowerCaseName)) {
+ return lowerCaseName;
+}
+// Fall through: Set iteration
+for (String candidate: candidates) {
+ if (candidate.equalsIgnoreCase(name)) {
+return candidate;
+ }
+}
+return null;
+ }
+
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+if (caseSensitive) {
+ Schema s = schema.getSubSchema(schemaName);
+ if (s != null) {
+return new SimpleCalciteSchema(this, s, schemaName);
+ }
+} else {
+ final String schemaName2 = caseInsensitiveLookup(
+ schema.getSubSchemaNames(), schemaName);
+ if (schemaName2 != null) {
+return new SimpleCalciteSchema(this,
+schema.getSubSchema(schemaName2), schemaName2);
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r378184129
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -67,31 +68,64 @@ public CalciteSchema add(String name, Schema schema) {
return calciteSchema;
}
+ private String caseInsensitiveLookup(Set candidates, String name) {
+for (String candidate: candidates) {
Review comment:
I agree. It makes sense for most frequent cases, i.e., upper case or lower
case.
I will add this optimization soon. Thanks!
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r377542184
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -70,28 +69,29 @@ public CalciteSchema add(String name, Schema schema) {
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final NameSet names = NameSet.immutableCopyOf(schema.getSubSchemaNames());
+for (String schemaName2: names.range(schemaName, caseSensitive)) {
+ return new SimpleCalciteSchema(this,
+ schema.getSubSchema(schemaName2), schemaName);
}
return null;
}
protected TableEntry getImplicitTable(String tableName,
boolean caseSensitive) {
// Check implicit tables.
-Table table = schema.getTable(tableName);
-if (table != null) {
- return tableEntry(tableName, table);
+final NameSet names = NameSet.immutableCopyOf(schema.getTableNames());
+for (String tableName2: names.range(tableName, caseSensitive)) {
Review comment:
Thanks! I see, ignore the above comment.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r377520403
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -70,28 +69,29 @@ public CalciteSchema add(String name, Schema schema) {
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final NameSet names = NameSet.immutableCopyOf(schema.getSubSchemaNames());
+for (String schemaName2: names.range(schemaName, caseSensitive)) {
+ return new SimpleCalciteSchema(this,
+ schema.getSubSchema(schemaName2), schemaName);
}
return null;
}
protected TableEntry getImplicitTable(String tableName,
boolean caseSensitive) {
// Check implicit tables.
-Table table = schema.getTable(tableName);
-if (table != null) {
- return tableEntry(tableName, table);
+final NameSet names = NameSet.immutableCopyOf(schema.getTableNames());
+for (String tableName2: names.range(tableName, caseSensitive)) {
Review comment:
Could you elaborate it? Or do you mean enumerating all possibilities?
E.g., converting `getTable("xy", false) ` to
`getTable("XY") + getTable("Xy") + getTable("xY") +getTable("xy")`?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r377422192
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -70,28 +69,29 @@ public CalciteSchema add(String name, Schema schema) {
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final NameSet names = NameSet.immutableCopyOf(schema.getSubSchemaNames());
+for (String schemaName2: names.range(schemaName, caseSensitive)) {
+ return new SimpleCalciteSchema(this,
+ schema.getSubSchema(schemaName2), schemaName);
}
return null;
}
protected TableEntry getImplicitTable(String tableName,
boolean caseSensitive) {
// Check implicit tables.
-Table table = schema.getTable(tableName);
-if (table != null) {
- return tableEntry(tableName, table);
+final NameSet names = NameSet.immutableCopyOf(schema.getTableNames());
+for (String tableName2: names.range(tableName, caseSensitive)) {
Review comment:
Thanks, you are right, the performance is not acceptable.
We should not copy them and use `NameSet` as that in `CachingCalciteSchema`.
I implement a `lookup` method and make some performance tests in my local
environment.
```
tables NameSetlookup
10 224ms 9ms
100 1639ms21ms
1000 14069ms117ms
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r377422192
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -70,28 +69,29 @@ public CalciteSchema add(String name, Schema schema) {
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final NameSet names = NameSet.immutableCopyOf(schema.getSubSchemaNames());
+for (String schemaName2: names.range(schemaName, caseSensitive)) {
+ return new SimpleCalciteSchema(this,
+ schema.getSubSchema(schemaName2), schemaName);
}
return null;
}
protected TableEntry getImplicitTable(String tableName,
boolean caseSensitive) {
// Check implicit tables.
-Table table = schema.getTable(tableName);
-if (table != null) {
- return tableEntry(tableName, table);
+final NameSet names = NameSet.immutableCopyOf(schema.getTableNames());
+for (String tableName2: names.range(tableName, caseSensitive)) {
Review comment:
Thanks, you are right, the performance is not acceptable.
We should not copy them and use `NameSet` as that in `CachingCalciteSchema`.
I implement a `lookup` method and make some performance tests in my local
environment.
```
tables NameSetlookup
10 224ms9ms
100 1639ms21ms
1000 14069ms117ms
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r377422192
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -70,28 +69,29 @@ public CalciteSchema add(String name, Schema schema) {
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final NameSet names = NameSet.immutableCopyOf(schema.getSubSchemaNames());
+for (String schemaName2: names.range(schemaName, caseSensitive)) {
+ return new SimpleCalciteSchema(this,
+ schema.getSubSchema(schemaName2), schemaName);
}
return null;
}
protected TableEntry getImplicitTable(String tableName,
boolean caseSensitive) {
// Check implicit tables.
-Table table = schema.getTable(tableName);
-if (table != null) {
- return tableEntry(tableName, table);
+final NameSet names = NameSet.immutableCopyOf(schema.getTableNames());
+for (String tableName2: names.range(tableName, caseSensitive)) {
Review comment:
Thanks, you are right, the performance is not acceptable.
We should not copy them and use `NameSet` as that in `CachingCalciteSchema`.
I implement a `lookup` method and make some performance tests in my local
environment.
```
tables NameSetlookup
10 224ms9ms
100 1639ms 21ms
1000 14069ms117ms
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r377422192
##
File path: core/src/main/java/org/apache/calcite/jdbc/SimpleCalciteSchema.java
##
@@ -70,28 +69,29 @@ public CalciteSchema add(String name, Schema schema) {
protected CalciteSchema getImplicitSubSchema(String schemaName,
boolean caseSensitive) {
// Check implicit schemas.
-Schema s = schema.getSubSchema(schemaName);
-if (s != null) {
- return new SimpleCalciteSchema(this, s, schemaName);
+final NameSet names = NameSet.immutableCopyOf(schema.getSubSchemaNames());
+for (String schemaName2: names.range(schemaName, caseSensitive)) {
+ return new SimpleCalciteSchema(this,
+ schema.getSubSchema(schemaName2), schemaName);
}
return null;
}
protected TableEntry getImplicitTable(String tableName,
boolean caseSensitive) {
// Check implicit tables.
-Table table = schema.getTable(tableName);
-if (table != null) {
- return tableEntry(tableName, table);
+final NameSet names = NameSet.immutableCopyOf(schema.getTableNames());
+for (String tableName2: names.range(tableName, caseSensitive)) {
Review comment:
Thanks, you are right, the performance is not acceptable.
We should not copy them and use `NameSet` as that in `CachingCalciteSchema`.
I implement a `lookup` method and make some performance tests in my local
environment.
```
tablesNameSetlookup
10 224ms9ms
100 1639ms 21ms
1000 14069ms117ms
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
DonnyZone commented on a change in pull request #1792: [CALCITE-3775] Implicit
lookup methods in SimpleCalciteSchema ignore case sensitivity parameter
URL: https://github.com/apache/calcite/pull/1792#discussion_r377421855
##
File path: core/src/test/java/org/apache/calcite/test/JdbcTest.java
##
@@ -6616,6 +6617,43 @@ private void checkGetTimestamp(Connection con) throws
SQLException {
assertThat(aSchema.getSubSchemaNames().size(), is(2));
}
+ @Test public void testCaseSensitiveConfigurableSimpleCalciteSchema() throws
Exception {
+final SchemaPlus rootSchema = CalciteSchema.createRootSchema(false,
false).plus();
+// create schema "/a"
+final Map dummySubSchemaMap = new HashMap<>();
+final Map dummyTableMap = new HashMap<>();
+final Map dummyTypeMap = new HashMap<>();
+final SchemaPlus dummySchema = rootSchema.add("dummy",
+new AbstractSchema() {
+ @Override protected Map getSubSchemaMap() {
+return dummySubSchemaMap;
+ }
+
+ @Override protected Map getTableMap() {
+return dummyTableMap;
+ }
+
+ @Override protected Map getTypeMap() {
+return dummyTypeMap;
+ }
+});
+// add implicit schema "/dummy/abc"
+dummySubSchemaMap.put("abc", new AbstractSchema());
+// add implicit table "/dummy/xyz"
+dummyTableMap.put("xyz", new AbstractTable() {
+ @Override public RelDataType getRowType(RelDataTypeFactory typeFactory) {
+return null;
+ }
+});
+// add implicit table "/dummy/myType"
+dummyTypeMap.put("myType", factory -> factory.builder().build());
+
+final CalciteSchema dummyCalciteSchema = CalciteSchema.from(dummySchema);
+assertThat(dummyCalciteSchema.getSubSchema("aBC", false), notNullValue());
+assertThat(dummyCalciteSchema.getTable("XyZ", false), notNullValue());
+assertThat(dummyCalciteSchema.getType("MytYpE", false), notNullValue());
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
