You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2023/01/07 13:04:00 UTC
[GitHub] [drill] vvysotskyi opened a new pull request, #2733: DRILL-8380: Remove customised SqlValidatorImpl.deriveAlias
vvysotskyi opened a new pull request, #2733:
URL: https://github.com/apache/drill/pull/2733
# [DRILL-8380](https://issues.apache.org/jira/browse/DRILL-8380): Remove customised SqlValidatorImpl.deriveAlias
## Description
As pointed out in CALCITE-5463, `SqlValidatorImpl.deriveAlias` isn't intended to be customized, and it is not used in the latest version. It causes issues with table and storage aliases and temporary tables functionality.
Unfortunately, there is no way to preserve existing temporary table behavior. After these changes, the temporary table will be accessible only within its workspace, as regular tables.
## Documentation
Yes.
## Testing
All UT pass.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on pull request #2733: DRILL-8380: Remove customised SqlValidatorImpl.deriveAlias
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on PR #2733:
URL: https://github.com/apache/drill/pull/2733#issuecomment-1384305483
I think Splunk tests have somewhere a condition to fail if I'm the author of the commit 😅
Here is CI run for another my commit in the master branch that has the same error: https://github.com/apache/drill/actions/runs/3874925191/jobs/6620797212
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre merged pull request #2733: DRILL-8380: Remove customised SqlValidatorImpl.deriveAlias
Posted by GitBox <gi...@apache.org>.
cgivre merged PR #2733:
URL: https://github.com/apache/drill/pull/2733
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on a diff in pull request #2733: DRILL-8380: Remove customised SqlValidatorImpl.deriveAlias
Posted by GitBox <gi...@apache.org>.
jnturton commented on code in PR #2733:
URL: https://github.com/apache/drill/pull/2733#discussion_r1067718395
##########
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/WorkspaceSchemaFactory.java:
##########
@@ -403,8 +404,24 @@ private View getView(DotDrillFile f) throws IOException {
return f.getView(mapper);
}
+ private String getTemporaryName(String name) {
+ if (isTemporaryWorkspace()) {
+ String tableName = DrillStringUtils.removeLeadingSlash(name);
+ return schemaConfig.getTemporaryTableName(tableName);
+ }
+ return null;
+ }
+
+ private boolean isTemporaryWorkspace() {
Review Comment:
Could this utiltiy method move to SchemaConfig or SchemaUtilities so that it's available for reuse elsewhere or is that unlikely?
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on a diff in pull request #2733: DRILL-8380: Remove customised SqlValidatorImpl.deriveAlias
Posted by GitBox <gi...@apache.org>.
jnturton commented on code in PR #2733:
URL: https://github.com/apache/drill/pull/2733#discussion_r1068252216
##########
exec/java-exec/src/main/java/org/apache/drill/exec/work/metadata/MetadataProvider.java:
##########
@@ -607,6 +608,16 @@ public String getQueryUserName() {
@Override public UserCredentials getQueryUserCredentials() {
return session.getCredentials();
}
+
+ @Override
+ public String getTemporaryTableName(String table) {
Review Comment:
This looks like another case where we wouldn't need to keep expanding interfaces like SchemaConfigInfoProvider and adding partial implementations where some throw UnsupportedOperationExceptions if we just had a good way of accessing the UserSession from most layers of Drill. It's not something for this PR for sure, but I wanted to remark to get your opinion since I remember having to work the same way when I was trying to expose UserCredentials (visible above) for user translation in plugins.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/work/metadata/MetadataProvider.java:
##########
@@ -607,6 +608,16 @@ public String getQueryUserName() {
@Override public UserCredentials getQueryUserCredentials() {
return session.getCredentials();
}
+
+ @Override
+ public String getTemporaryTableName(String table) {
+ return session.resolveTemporaryTableName(table);
+ }
+
+ @Override
+ public String getTemporaryWorkspace() {
+ return config.getString(ExecConstants.DEFAULT_TEMPORARY_WORKSPACE);
Review Comment:
Have I got it right that this config option value is the only value returned by implementations of getTemporaryWorkspace? If so, do we need this method or could its callers look up the config value themselves instead?
##########
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/conversion/DrillCalciteCatalogReader.java:
##########
@@ -135,14 +112,15 @@ public Prepare.PreparingTable getTable(List<String> names) {
}
private void checkTemporaryTable(List<String> names) {
- if (allowTemporaryTables) {
+ if (allowTemporaryTables || !needsTemporaryTableCheck(names, session.getDefaultSchemaPath(), drillConfig)) {
return;
}
- String originalTableName = session.getOriginalTableNameFromTemporaryTable(names.get(names.size() - 1));
+ String tableName = names.get(names.size() - 1);
+ String originalTableName = session.resolveTemporaryTableName(tableName);
if (originalTableName != null) {
throw UserException
.validationError()
- .message("Temporary tables usage is disallowed. Used temporary table name: [%s].", originalTableName)
+ .message("Temporary tables usage is disallowed. Used temporary table name: [%s].", tableName)
Review Comment:
```suggestion
.message("A reference to temporary table [%s] was made in a context where temporary table references are not allowed.", tableName)
```
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2733: DRILL-8380: Remove customised SqlValidatorImpl.deriveAlias
Posted by GitBox <gi...@apache.org>.
cgivre commented on PR #2733:
URL: https://github.com/apache/drill/pull/2733#issuecomment-1384307651
One thing I noticed is that the splunk tests sometimes fail locally as they don't have results.clear() at the end. This is inconsistent behavior, but I added that in the ES PR that I'm working on.
Best,
-- C
> On Jan 16, 2023, at 11:39 AM, Volodymyr Vysotskyi ***@***.***> wrote:
>
> hat
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on a diff in pull request #2733: DRILL-8380: Remove customised SqlValidatorImpl.deriveAlias
Posted by GitBox <gi...@apache.org>.
jnturton commented on code in PR #2733:
URL: https://github.com/apache/drill/pull/2733#discussion_r1067718395
##########
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/WorkspaceSchemaFactory.java:
##########
@@ -403,8 +404,24 @@ private View getView(DotDrillFile f) throws IOException {
return f.getView(mapper);
}
+ private String getTemporaryName(String name) {
+ if (isTemporaryWorkspace()) {
+ String tableName = DrillStringUtils.removeLeadingSlash(name);
+ return schemaConfig.getTemporaryTableName(tableName);
+ }
+ return null;
+ }
+
+ private boolean isTemporaryWorkspace() {
Review Comment:
Could this utility method move to SchemaConfig or SchemaUtilities so that it's available for reuse elsewhere or is that unlikely?
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] jnturton commented on pull request #2733: DRILL-8380: Remove customised SqlValidatorImpl.deriveAlias
Posted by GitBox <gi...@apache.org>.
jnturton commented on PR #2733:
URL: https://github.com/apache/drill/pull/2733#issuecomment-1383868835
@vvysotskyi given that most of the CI runs had passed I ran (8, default-hadoop) again but the same failure turned up. I wouldn't expect a failure such as the following to affect only JDK 8 though so I'm a little mystified.
```
Error: SplunkWriterTest.testBasicCTASWithScalarDataTypes:136 Schemas don't match.
Expected: [TupleSchema [PrimitiveColumnMetadata [`int_field` (VARCHAR:OPTIONAL)]], [PrimitiveColumnMetadata [`bigint_field` (VARCHAR:OPTIONAL)]], [PrimitiveColumnMetadata [`float4_field` (VARCHAR:OPTIONAL)]], [PrimitiveColumnMetadata [`float8_field` (VARCHAR:OPTIONAL)]], [PrimitiveColumnMetadata [`varchar_field` (VARCHAR:OPTIONAL)]], [PrimitiveColumnMetadata [`date_field` (VARCHAR:OPTIONAL)]], [PrimitiveColumnMetadata [`time_field` (VARCHAR:OPTIONAL)]], [PrimitiveColumnMetadata [`timestamp_field` (VARCHAR:OPTIONAL)]], [PrimitiveColumnMetadata [`boolean_field` (VARCHAR:OPTIONAL)]]]
Actual: [TupleSchema [PrimitiveColumnMetadata [`int_field` (INT:OPTIONAL)]], [PrimitiveColumnMetadata [`bigint_field` (INT:OPTIONAL)]], [PrimitiveColumnMetadata [`float4_field` (INT:OPTIONAL)]], [PrimitiveColumnMetadata [`float8_field` (INT:OPTIONAL)]], [PrimitiveColumnMetadata [`varchar_field` (INT:OPTIONAL)]], [PrimitiveColumnMetadata [`date_field` (INT:OPTIONAL)]], [PrimitiveColumnMetadata [`time_field` (INT:OPTIONAL)]], [PrimitiveColumnMetadata [`timestamp_field` (INT:OPTIONAL)]], [PrimitiveColumnMetadata [`boolean_field` (INT:OPTIONAL)]]]
```
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on a diff in pull request #2733: DRILL-8380: Remove customised SqlValidatorImpl.deriveAlias
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on code in PR #2733:
URL: https://github.com/apache/drill/pull/2733#discussion_r1070571396
##########
exec/java-exec/src/main/java/org/apache/drill/exec/store/dfs/WorkspaceSchemaFactory.java:
##########
@@ -403,8 +404,24 @@ private View getView(DotDrillFile f) throws IOException {
return f.getView(mapper);
}
+ private String getTemporaryName(String name) {
+ if (isTemporaryWorkspace()) {
+ String tableName = DrillStringUtils.removeLeadingSlash(name);
+ return schemaConfig.getTemporaryTableName(tableName);
+ }
+ return null;
+ }
+
+ private boolean isTemporaryWorkspace() {
Review Comment:
I think it is unlikely that it would be reused.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/work/metadata/MetadataProvider.java:
##########
@@ -607,6 +608,16 @@ public String getQueryUserName() {
@Override public UserCredentials getQueryUserCredentials() {
return session.getCredentials();
}
+
+ @Override
+ public String getTemporaryTableName(String table) {
Review Comment:
I agree it is not good to have such interfaces with unsupported methods. Ideally, we should split them into several interfaces instead and use broader ones in places where it is required.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/conversion/DrillCalciteCatalogReader.java:
##########
@@ -135,14 +112,15 @@ public Prepare.PreparingTable getTable(List<String> names) {
}
private void checkTemporaryTable(List<String> names) {
- if (allowTemporaryTables) {
+ if (allowTemporaryTables || !needsTemporaryTableCheck(names, session.getDefaultSchemaPath(), drillConfig)) {
return;
}
- String originalTableName = session.getOriginalTableNameFromTemporaryTable(names.get(names.size() - 1));
+ String tableName = names.get(names.size() - 1);
+ String originalTableName = session.resolveTemporaryTableName(tableName);
if (originalTableName != null) {
throw UserException
.validationError()
- .message("Temporary tables usage is disallowed. Used temporary table name: [%s].", originalTableName)
+ .message("Temporary tables usage is disallowed. Used temporary table name: [%s].", tableName)
Review Comment:
Thanks, replaced it.
##########
exec/java-exec/src/main/java/org/apache/drill/exec/work/metadata/MetadataProvider.java:
##########
@@ -607,6 +608,16 @@ public String getQueryUserName() {
@Override public UserCredentials getQueryUserCredentials() {
return session.getCredentials();
}
+
+ @Override
+ public String getTemporaryTableName(String table) {
+ return session.resolveTemporaryTableName(table);
+ }
+
+ @Override
+ public String getTemporaryWorkspace() {
+ return config.getString(ExecConstants.DEFAULT_TEMPORARY_WORKSPACE);
Review Comment:
Yes, config is the only source for this property. But I think it is better to have an interface that provides only information related to schema config info rather than allow callers to access config by themselves. The current approach helps to encapsulate it, so I would prefer to leave it as it is.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vvysotskyi commented on pull request #2733: DRILL-8380: Remove customised SqlValidatorImpl.deriveAlias
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on PR #2733:
URL: https://github.com/apache/drill/pull/2733#issuecomment-1384890894
Yes, it can be merged before Calcite is released.
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2733: DRILL-8380: Remove customised SqlValidatorImpl.deriveAlias
Posted by GitBox <gi...@apache.org>.
cgivre commented on PR #2733:
URL: https://github.com/apache/drill/pull/2733#issuecomment-1384683174
@vvysotskyi Can we merge this or should we wait for Calcite 1.33 to be released?
--
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: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org