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 2020/03/28 00:00:20 UTC
[GitHub] [drill] vvysotskyi opened a new pull request #2043: DRILL-7673:
View set query fails with NPE for non-existing option
vvysotskyi opened a new pull request #2043: DRILL-7673: View set query fails with NPE for non-existing option
URL: https://github.com/apache/drill/pull/2043
# [DRILL-7673](https://issues.apache.org/jira/browse/DRILL-7673): View set query fails with NPE for non-existing option
## Description
`OptionManager.getOption()` method returns null for non-existing options.
Added call for `OptionManager.getOptionDefinition()` whith throws valid exception for such case.
Used option name from `OptionDefinition` to display option name in the correct casing.
## Documentation
NA
## Testing
Added unit test.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #2043:
DRILL-7673: View set query fails with NPE for non-existing option
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2043: DRILL-7673: View set query fails with NPE for non-existing option
URL: https://github.com/apache/drill/pull/2043#discussion_r399613760
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java
##########
@@ -63,9 +64,10 @@ public final PhysicalPlan getPlan(SqlNode sqlNode) throws ForemanSetupException
SqlNode optionValue = statement.getValue();
if (optionValue == null) {
+ OptionDefinition optionDefinition = optionManager.getOptionDefinition(optionName);
Review comment:
Probably worth a comment saying that this call will validate the option to ensure it is valid.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on issue #2043: DRILL-7673: View
set query fails with NPE for non-existing option
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on issue #2043: DRILL-7673: View set query fails with NPE for non-existing option
URL: https://github.com/apache/drill/pull/2043#issuecomment-605453124
+1
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2043:
DRILL-7673: View set query fails with NPE for non-existing option
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2043: DRILL-7673: View set query fails with NPE for non-existing option
URL: https://github.com/apache/drill/pull/2043#discussion_r399648044
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java
##########
@@ -63,9 +64,10 @@ public final PhysicalPlan getPlan(SqlNode sqlNode) throws ForemanSetupException
SqlNode optionValue = statement.getValue();
if (optionValue == null) {
+ OptionDefinition optionDefinition = optionManager.getOptionDefinition(optionName);
Review comment:
Thanks, added a 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #2043:
DRILL-7673: View set query fails with NPE for non-existing option
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #2043: DRILL-7673: View set query fails with NPE for non-existing option
URL: https://github.com/apache/drill/pull/2043#discussion_r399613716
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java
##########
@@ -63,9 +64,10 @@ public final PhysicalPlan getPlan(SqlNode sqlNode) throws ForemanSetupException
SqlNode optionValue = statement.getValue();
if (optionValue == null) {
+ OptionDefinition optionDefinition = optionManager.getOptionDefinition(optionName);
String value = String.valueOf(optionManager.getOption(optionName).getValue());
- return DirectPlan.createDirectPlan(context, new SetOptionViewResult(optionName, value));
+ return DirectPlan.createDirectPlan(context, new SetOptionViewResult(optionDefinition.getValidator().getOptionName(), value));
Review comment:
The naive reader (that would be me) would wonder why not use the `optionName` rather than using this indirect approach. Maybe a brief comment saying that we're doing it this way to use the name as defined in the option, rather than what the user provided.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2043:
DRILL-7673: View set query fails with NPE for non-existing option
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2043: DRILL-7673: View set query fails with NPE for non-existing option
URL: https://github.com/apache/drill/pull/2043#discussion_r399664041
##########
File path: exec/java-exec/src/test/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandlerTest.java
##########
@@ -59,32 +63,41 @@ public void testSimpleSetQuery() throws Exception {
@Test
public void testViewSetQuery() throws Exception {
- client.testBuilder() // BIT
+ testBuilder() // BIT
.sqlQuery("SET `%s`", ExecConstants.ENABLE_ITERATOR_VALIDATION_OPTION)
.unOrdered()
.sqlBaselineQuery("SELECT name, val as value FROM sys.options where name = '%s' limit 1",
ExecConstants.ENABLE_ITERATOR_VALIDATION_OPTION)
.go();
- client.testBuilder() // BIGINT
+ testBuilder() // BIGINT
.sqlQuery("SET `%s`", ExecConstants.OUTPUT_BATCH_SIZE)
.unOrdered()
.sqlBaselineQuery("SELECT name, val as value FROM sys.options where name = '%s' limit 1",
ExecConstants.OUTPUT_BATCH_SIZE)
.go();
- client.testBuilder() // FLOAT
+ testBuilder() // FLOAT
.sqlQuery("SET `%s`", ExecConstants.OUTPUT_BATCH_SIZE_AVAIL_MEM_FACTOR)
.unOrdered()
.sqlBaselineQuery("SELECT name, val as value FROM sys.options where name = '%s' limit 1",
ExecConstants.OUTPUT_BATCH_SIZE_AVAIL_MEM_FACTOR)
.go();
- client.testBuilder() // VARCHAR
+ testBuilder() // VARCHAR
.sqlQuery("SET `%s`", ExecConstants.FILESYSTEM_PARTITION_COLUMN_LABEL)
.unOrdered()
.sqlBaselineQuery("SELECT name, val as value FROM sys.options where name = '%s' limit 1",
ExecConstants.FILESYSTEM_PARTITION_COLUMN_LABEL)
.go();
}
+
+ @Test
+ public void testViewSetWithIncorrectOption() throws Exception {
+ try {
+ run("set `non-existing option`");
Review comment:
Sorry, I forgot to add it. Thanks for pointing, added.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request
#2043: DRILL-7673: View set query fails with NPE for non-existing option
Posted by GitBox <gi...@apache.org>.
arina-ielchiieva commented on a change in pull request #2043: DRILL-7673: View set query fails with NPE for non-existing option
URL: https://github.com/apache/drill/pull/2043#discussion_r399662074
##########
File path: exec/java-exec/src/test/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandlerTest.java
##########
@@ -59,32 +63,41 @@ public void testSimpleSetQuery() throws Exception {
@Test
public void testViewSetQuery() throws Exception {
- client.testBuilder() // BIT
+ testBuilder() // BIT
.sqlQuery("SET `%s`", ExecConstants.ENABLE_ITERATOR_VALIDATION_OPTION)
.unOrdered()
.sqlBaselineQuery("SELECT name, val as value FROM sys.options where name = '%s' limit 1",
ExecConstants.ENABLE_ITERATOR_VALIDATION_OPTION)
.go();
- client.testBuilder() // BIGINT
+ testBuilder() // BIGINT
.sqlQuery("SET `%s`", ExecConstants.OUTPUT_BATCH_SIZE)
.unOrdered()
.sqlBaselineQuery("SELECT name, val as value FROM sys.options where name = '%s' limit 1",
ExecConstants.OUTPUT_BATCH_SIZE)
.go();
- client.testBuilder() // FLOAT
+ testBuilder() // FLOAT
.sqlQuery("SET `%s`", ExecConstants.OUTPUT_BATCH_SIZE_AVAIL_MEM_FACTOR)
.unOrdered()
.sqlBaselineQuery("SELECT name, val as value FROM sys.options where name = '%s' limit 1",
ExecConstants.OUTPUT_BATCH_SIZE_AVAIL_MEM_FACTOR)
.go();
- client.testBuilder() // VARCHAR
+ testBuilder() // VARCHAR
.sqlQuery("SET `%s`", ExecConstants.FILESYSTEM_PARTITION_COLUMN_LABEL)
.unOrdered()
.sqlBaselineQuery("SELECT name, val as value FROM sys.options where name = '%s' limit 1",
ExecConstants.FILESYSTEM_PARTITION_COLUMN_LABEL)
.go();
}
+
+ @Test
+ public void testViewSetWithIncorrectOption() throws Exception {
+ try {
+ run("set `non-existing option`");
Review comment:
Add fail() to ensure method call has failed...
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #2043:
DRILL-7673: View set query fails with NPE for non-existing option
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #2043: DRILL-7673: View set query fails with NPE for non-existing option
URL: https://github.com/apache/drill/pull/2043#discussion_r399648100
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/handlers/SetOptionHandler.java
##########
@@ -63,9 +64,10 @@ public final PhysicalPlan getPlan(SqlNode sqlNode) throws ForemanSetupException
SqlNode optionValue = statement.getValue();
if (optionValue == null) {
+ OptionDefinition optionDefinition = optionManager.getOptionDefinition(optionName);
String value = String.valueOf(optionManager.getOption(optionName).getValue());
- return DirectPlan.createDirectPlan(context, new SetOptionViewResult(optionName, value));
+ return DirectPlan.createDirectPlan(context, new SetOptionViewResult(optionDefinition.getValidator().getOptionName(), value));
Review comment:
Thanks, added a comment with 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] asfgit closed pull request #2043: DRILL-7673: View set
query fails with NPE for non-existing option
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #2043: DRILL-7673: View set query fails with NPE for non-existing option
URL: https://github.com/apache/drill/pull/2043
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services