[jira] [Commented] (DRILL-3497) Throw UserException#validationError for errors when modifying options
[ https://issues.apache.org/jira/browse/DRILL-3497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14746090#comment-14746090 ] Sudheesh Katkam commented on DRILL-3497: Invalid parameters to "ALTER ... SET ..." command should throw exceptions with specific type (e.g. VALIDATION_ERROR) instead of SYSTEM_ERROR. > Throw UserException#validationError for errors when modifying options > - > > Key: DRILL-3497 > URL: https://issues.apache.org/jira/browse/DRILL-3497 > Project: Apache Drill > Issue Type: Bug > Components: Execution - Flow >Reporter: Sudheesh Katkam >Assignee: Sudheesh Katkam >Priority: Minor > Fix For: 1.2.0 > > -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (DRILL-3497) Throw UserException#validationError for errors when modifying options
[ https://issues.apache.org/jira/browse/DRILL-3497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14735218#comment-14735218 ] ASF GitHub Bot commented on DRILL-3497: --- Github user asfgit closed the pull request at: https://github.com/apache/drill/pull/98 > Throw UserException#validationError for errors when modifying options > - > > Key: DRILL-3497 > URL: https://issues.apache.org/jira/browse/DRILL-3497 > Project: Apache Drill > Issue Type: Bug > Components: Execution - Flow >Reporter: Sudheesh Katkam >Assignee: Jason Altekruse >Priority: Minor > Fix For: 1.2.0 > > -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (DRILL-3497) Throw UserException#validationError for errors when modifying options
[
https://issues.apache.org/jira/browse/DRILL-3497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14729288#comment-14729288
]
ASF GitHub Bot commented on DRILL-3497:
---
Github user jaltekruse commented on a diff in the pull request:
https://github.com/apache/drill/pull/98#discussion_r38660966
--- Diff:
exec/java-exec/src/test/java/org/apache/drill/exec/testing/ControlsInjectionUtil.java
---
@@ -74,7 +75,7 @@ public static void setControls(final UserSession session,
final String controls)
final OptionManager options = session.getOptions();
try {
- options.getAdmin().validate(opValue);
+
SystemOptionManager.getValidator(DRILLBIT_CONTROL_INJECTIONS).validate(opValue);
--- End diff --
This could just be a reference to the validator itself, as it is also
public and static, rather than looking it up by name.
> Throw UserException#validationError for errors when modifying options
> -
>
> Key: DRILL-3497
> URL: https://issues.apache.org/jira/browse/DRILL-3497
> Project: Apache Drill
> Issue Type: Bug
> Components: Execution - Flow
>Reporter: Sudheesh Katkam
>Assignee: Jason Altekruse
>Priority: Minor
> Fix For: 1.2.0
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (DRILL-3497) Throw UserException#validationError for errors when modifying options
[
https://issues.apache.org/jira/browse/DRILL-3497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14729718#comment-14729718
]
ASF GitHub Bot commented on DRILL-3497:
---
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/98#discussion_r38692395
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java
---
@@ -35,61 +42,65 @@ public FallbackOptionManager(OptionManager fallback) {
@Override
public Iterator iterator() {
-return Iterables.concat(fallback, optionIterable()).iterator();
+return Iterables.concat(fallback, getLocalOptions()).iterator();
}
@Override
- public OptionValue getOption(String name) {
-final OptionValue opt = getLocalOption(name);
-if(opt == null && fallback != null){
+ public OptionValue getOption(final String name) {
+final OptionValue value = getLocalOption(name);
+if (value == null && fallback != null) {
--- End diff --
Will add a precondition.
> Throw UserException#validationError for errors when modifying options
> -
>
> Key: DRILL-3497
> URL: https://issues.apache.org/jira/browse/DRILL-3497
> Project: Apache Drill
> Issue Type: Bug
> Components: Execution - Flow
>Reporter: Sudheesh Katkam
>Assignee: Jason Altekruse
>Priority: Minor
> Fix For: 1.2.0
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (DRILL-3497) Throw UserException#validationError for errors when modifying options
[
https://issues.apache.org/jira/browse/DRILL-3497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14729717#comment-14729717
]
ASF GitHub Bot commented on DRILL-3497:
---
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/98#discussion_r38692377
--- Diff:
common/src/main/java/org/apache/drill/common/map/CaseInsensitiveMap.java ---
@@ -0,0 +1,141 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.common.map;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Maps;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * A special type of {@link Map} with {@link String}s as keys, and the
case of a key is ignored for operations involving
+ * keys like {@link #put}, {@link #get}, etc. The keys are stored and
retrieved in lower case. Use the static methods to
+ * create instances of this class (e.g. {@link #newConcurrentMap}).
+ *
+ * @param the type of values to be stored in the map
+ */
+public class CaseInsensitiveMap implements Map {
+
+ /**
+ * Returns a new instance of {@link java.util.concurrent.ConcurrentMap}
with key case-insensitivity. See
+ * {@link java.util.concurrent.ConcurrentMap}.
+ *
+ * @param type of values to be stored in the map
+ * @return key case-insensitive concurrent map
+ */
+ public static CaseInsensitiveMap newConcurrentMap() {
+return new CaseInsensitiveMap<>(Maps.newConcurrentMap());
+ }
+
+ /**
+ * Returns a new instance of {@link java.util.HashMap} with key
case-insensitivity. See {@link java.util.HashMap}.
+ *
+ * @param type of values to be stored in the map
+ * @return key case-insensitive hash map
+ */
+ public static CaseInsensitiveMap newHashMap() {
+return new CaseInsensitiveMap<>(Maps.newHashMap());
+ }
+
+ /**
+ * Returns a new instance of {@link ImmutableMap} with key
case-insensitivity. This map is built from the given
+ * map. See {@link ImmutableMap}.
+ *
+ * @param map map to copy from
+ * @param type of values to be stored in the map
+ * @return key case-insensitive immutable map
+ */
+ public static CaseInsensitiveMap newImmutableMap(final
Map map) {
+final ImmutableMap.Builder builder =
ImmutableMap.builder();
+for (final Entry entry :
map.entrySet()) {
+ builder.put(entry.getKey().toLowerCase(), entry.getValue());
+}
+return new CaseInsensitiveMap<>(builder.build());
+ }
+
+ private final Map underlyingMap;
+
+ protected CaseInsensitiveMap(final Map underlyingMap) {
+this.underlyingMap = underlyingMap;
--- End diff --
That's not possible because the ctor is protected. I'll make the ctor
private, and add appropriate comments.
> Throw UserException#validationError for errors when modifying options
> -
>
> Key: DRILL-3497
> URL: https://issues.apache.org/jira/browse/DRILL-3497
> Project: Apache Drill
> Issue Type: Bug
> Components: Execution - Flow
>Reporter: Sudheesh Katkam
>Assignee: Jason Altekruse
>Priority: Minor
> Fix For: 1.2.0
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (DRILL-3497) Throw UserException#validationError for errors when modifying options
[
https://issues.apache.org/jira/browse/DRILL-3497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14728044#comment-14728044
]
ASF GitHub Bot commented on DRILL-3497:
---
Github user jaltekruse commented on a diff in the pull request:
https://github.com/apache/drill/pull/98#discussion_r38586355
--- Diff:
common/src/main/java/org/apache/drill/common/map/CaseInsensitiveMap.java ---
@@ -0,0 +1,141 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.common.map;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Maps;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * A special type of {@link Map} with {@link String}s as keys, and the
case of a key is ignored for operations involving
+ * keys like {@link #put}, {@link #get}, etc. The keys are stored and
retrieved in lower case. Use the static methods to
+ * create instances of this class (e.g. {@link #newConcurrentMap}).
+ *
+ * @param the type of values to be stored in the map
+ */
+public class CaseInsensitiveMap implements Map {
+
+ /**
+ * Returns a new instance of {@link java.util.concurrent.ConcurrentMap}
with key case-insensitivity. See
+ * {@link java.util.concurrent.ConcurrentMap}.
+ *
+ * @param type of values to be stored in the map
+ * @return key case-insensitive concurrent map
+ */
+ public static CaseInsensitiveMap newConcurrentMap() {
+return new CaseInsensitiveMap<>(Maps.newConcurrentMap());
+ }
+
+ /**
+ * Returns a new instance of {@link java.util.HashMap} with key
case-insensitivity. See {@link java.util.HashMap}.
+ *
+ * @param type of values to be stored in the map
+ * @return key case-insensitive hash map
+ */
+ public static CaseInsensitiveMap newHashMap() {
+return new CaseInsensitiveMap<>(Maps.newHashMap());
+ }
+
+ /**
+ * Returns a new instance of {@link ImmutableMap} with key
case-insensitivity. This map is built from the given
+ * map. See {@link ImmutableMap}.
+ *
+ * @param map map to copy from
+ * @param type of values to be stored in the map
+ * @return key case-insensitive immutable map
+ */
+ public static CaseInsensitiveMap newImmutableMap(final
Map map) {
+final ImmutableMap.Builder builder =
ImmutableMap.builder();
+for (final Entry entry :
map.entrySet()) {
+ builder.put(entry.getKey().toLowerCase(), entry.getValue());
+}
+return new CaseInsensitiveMap<>(builder.build());
+ }
+
+ private final Map underlyingMap;
+
+ protected CaseInsensitiveMap(final Map underlyingMap) {
+this.underlyingMap = underlyingMap;
--- End diff --
Would it be worth enforcing that the map being wrapped is empty? As the
newImmuatableMap() method makes a copy of the map, properly importing the keys,
someone may get the wrong impression that this constructor would remove and
re-insert the values currently in the map so that they will be accessible.
Right now it would be possible to wrap a map that has uppercase in the key
names and those members would not be able to be extracted through this
interface.
> Throw UserException#validationError for errors when modifying options
> -
>
> Key: DRILL-3497
> URL: https://issues.apache.org/jira/browse/DRILL-3497
> Project: Apache Drill
> Issue Type: Bug
> Components: Execution - Flow
>Reporter: Sudheesh Katkam
>Assignee: Jason Altekruse
>Priority: Minor
> Fix For: 1.2.0
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (DRILL-3497) Throw UserException#validationError for errors when modifying options
[
https://issues.apache.org/jira/browse/DRILL-3497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14728165#comment-14728165
]
ASF GitHub Bot commented on DRILL-3497:
---
Github user jaltekruse commented on a diff in the pull request:
https://github.com/apache/drill/pull/98#discussion_r38594193
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/FallbackOptionManager.java
---
@@ -35,61 +42,65 @@ public FallbackOptionManager(OptionManager fallback) {
@Override
public Iterator iterator() {
-return Iterables.concat(fallback, optionIterable()).iterator();
+return Iterables.concat(fallback, getLocalOptions()).iterator();
}
@Override
- public OptionValue getOption(String name) {
-final OptionValue opt = getLocalOption(name);
-if(opt == null && fallback != null){
+ public OptionValue getOption(final String name) {
+final OptionValue value = getLocalOption(name);
+if (value == null && fallback != null) {
--- End diff --
This null check on the fallback manager seems odd to me. If something
doesn't have a fallback, why would they create one of these? Should a non-null
fallback be enforced in the constructor?
> Throw UserException#validationError for errors when modifying options
> -
>
> Key: DRILL-3497
> URL: https://issues.apache.org/jira/browse/DRILL-3497
> Project: Apache Drill
> Issue Type: Bug
> Components: Execution - Flow
>Reporter: Sudheesh Katkam
>Assignee: Jason Altekruse
>Priority: Minor
> Fix For: 1.2.0
>
>
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (DRILL-3497) Throw UserException#validationError for errors when modifying options
[ https://issues.apache.org/jira/browse/DRILL-3497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14637990#comment-14637990 ] ASF GitHub Bot commented on DRILL-3497: --- Github user sudheeshkatkam commented on the pull request: https://github.com/apache/drill/pull/98#issuecomment-123917913 Addressed review comments mentioned on #91 Throw UserException#validationError for errors when modifying options - Key: DRILL-3497 URL: https://issues.apache.org/jira/browse/DRILL-3497 Project: Apache Drill Issue Type: Bug Components: Execution - Flow Reporter: Sudheesh Katkam Assignee: Sudheesh Katkam Priority: Minor Fix For: 1.2.0 -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (DRILL-3497) Throw UserException#validationError for errors when modifying options
[ https://issues.apache.org/jira/browse/DRILL-3497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14637962#comment-14637962 ] ASF GitHub Bot commented on DRILL-3497: --- GitHub user sudheeshkatkam opened a pull request: https://github.com/apache/drill/pull/98 DRILL-3497: Throw UserException#validationError for errors when modifying options Mistakenly closed https://github.com/apache/drill/pull/91. Please review 4c2b698. You can merge this pull request into a Git repository by running: $ git pull https://github.com/sudheeshkatkam/drill DRILL-3497 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/98.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #98 commit 9dc056ec9f60b0e91acd345ca496a7e6fc077c05 Author: Sudheesh Katkam skat...@maprtech.com Date: 2015-07-22T23:32:02Z DRILL-2304: Manage options with lowercase names Major refactoring: + Removed unnecessary methods from OptionManagers, and refactored implementations + Removed OptionAdmin interface, which was a delegate to validators Other fixes: + Added CaseInsensitiveMap + Moved OptionValue creation from TypeValidators to SetOptionHandler + Fixed error message in BaseOptionManager + Added documentation for all OptionManagers, and OptionValidator commit 4c2b698fe5c059c7987c808e7e90f55659b74ba5 Author: Sudheesh Katkam skat...@maprtech.com Date: 2015-07-14T22:33:30Z DRILL-3497: Throw UserException#validationError instead of SetOptionException and ExpressionParsingException for options + Fixed bug in PositiveLongValidator (0 is not positive) + Added UserExceptionMatcher for unit tests Throw UserException#validationError for errors when modifying options - Key: DRILL-3497 URL: https://issues.apache.org/jira/browse/DRILL-3497 Project: Apache Drill Issue Type: Bug Components: Execution - Flow Reporter: Sudheesh Katkam Assignee: Sudheesh Katkam Priority: Minor Fix For: 1.2.0 -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (DRILL-3497) Throw UserException#validationError for errors when modifying options
[ https://issues.apache.org/jira/browse/DRILL-3497?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=14627197#comment-14627197 ] ASF GitHub Bot commented on DRILL-3497: --- GitHub user sudheeshkatkam opened a pull request: https://github.com/apache/drill/pull/91 DRILL-3497: Throw UserException#validationError for errors when modifying options Rebased on top of DRILL-2304. @jaltekruse please review the third commit: 6ec7b32 You can merge this pull request into a Git repository by running: $ git pull https://github.com/sudheeshkatkam/drill DRILL-3497 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/91.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #91 commit 313c8e3632fa71a5ed5f62baf1146e8c20f80065 Author: Sudheesh Katkam skat...@maprtech.com Date: 2015-07-14T18:29:56Z DRILL-2304: Part 1: Documentation for OptionManagers + corrected error message in BaseOptionManager + use getAdmin in setOption method commit f33654c7c539863e56958cc1746eded329b73e61 Author: Sudheesh Katkam skat...@maprtech.com Date: 2015-07-14T21:54:16Z DRILL-2304: Part 2: Manage options with lowercase names commit 6ec7b326459094a865846b1758e9d4457752af4c Author: Sudheesh Katkam skat...@maprtech.com Date: 2015-07-14T22:33:30Z DRILL-3497: Throw UserException#validationError instead of SetOptionException and ExpressionParsingException for options + Fixed bug in PositiveLongValidator (0 is not positive) Throw UserException#validationError for errors when modifying options - Key: DRILL-3497 URL: https://issues.apache.org/jira/browse/DRILL-3497 Project: Apache Drill Issue Type: Bug Components: Execution - Flow Reporter: Sudheesh Katkam Assignee: Sudheesh Katkam Priority: Minor Fix For: 1.2.0 -- This message was sent by Atlassian JIRA (v6.3.4#6332)
