You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by arina-ielchiieva <gi...@git.apache.org> on 2016/11/17 14:15:56 UTC
[GitHub] drill pull request #655: DRILL-5047: When session option is string, query pr...
GitHub user arina-ielchiieva opened a pull request:
https://github.com/apache/drill/pull/655
DRILL-5047: When session option is string, query profile is displayed\u2026
\u2026 incorrectly on Web UI
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/arina-ielchiieva/drill DRILL-5047
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/655.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 #655
----
commit 686d820ca4216f3da4d3570f9158223707b2259a
Author: Arina Ielchiieva <ar...@gmail.com>
Date: 2016-11-17T12:44:34Z
DRILL-5047: When session option is string, query profile is displayed incorrectly on Web UI
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #655: DRILL-5047: When session option is string, query profile i...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/655
Answering to all comments in one:
1. `${option.getValue()}` is not sufficient since method returns Object and freemarker does not know how to cast it.
2. `?c` only works with boolean and numbers, since options can be string, it not sufficient
3. `?string` works with most of the types but it's true for boolean seems to be deprecated
4. to set null value to option is not possible (handled in SetOptionHandler.class) but option.getValue() may return null according to the code.
Paul's idea to prepare options to display on Java side seems to be the most reasonable.
So I have updated the pull request, added new method in ProfileWrapper.class with transforms `OptionValueList` into `Map<String, String>`. If option value is null, it will be displayed as word 'null'.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #655: DRILL-5047: When session option is string, query profile i...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the issue:
https://github.com/apache/drill/pull/655
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #655: DRILL-5047: When session option is string, query pr...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on a diff in the pull request:
https://github.com/apache/drill/pull/655#discussion_r88602974
--- Diff: exec/java-exec/src/main/resources/rest/profile/profile.ftl ---
@@ -132,7 +132,7 @@
<#list model.getOptionList() as option>
<tr>
<td>${option.getName()}</td>
- <td>${option.getValue()?c}</td>
+ <td>${option.getValue()?string}</td>
--- End diff --
Tried this out. All the options take non-null values. Any attempt to set a property (String, for e.g.) to null failed. So, I guess as long as a property takes only non-null values, things should be fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #655: DRILL-5047: When session option is string, query pr...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/655#discussion_r88701851
--- Diff: exec/java-exec/src/main/resources/rest/profile/profile.ftl ---
@@ -129,10 +130,10 @@
</tr>
</thead>
<tbody>
- <#list model.getOptionList() as option>
+ <#list options?keys as name>
--- End diff --
Neither this nor the original design displays the keys as sorted. Sorted display is a fairly traditional to save the user from a random search for each value. Since we are changing the behavior, should we fix the sorting issue as well?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #655: DRILL-5047: When session option is string, query pr...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on a diff in the pull request:
https://github.com/apache/drill/pull/655#discussion_r88702773
--- Diff: exec/java-exec/src/main/resources/rest/profile/profile.ftl ---
@@ -129,10 +130,10 @@
</tr>
</thead>
<tbody>
- <#list model.getOptionList() as option>
+ <#list options?keys as name>
--- End diff --
Agree. I'll make fix shortly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #655: DRILL-5047: When session option is string, query pr...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/655#discussion_r88528313
--- Diff: exec/java-exec/src/main/resources/rest/profile/profile.ftl ---
@@ -132,7 +132,7 @@
<#list model.getOptionList() as option>
<tr>
<td>${option.getName()}</td>
- <td>${option.getValue()?c}</td>
+ <td>${option.getValue()?string}</td>
--- End diff --
For [the documentation](http://freemarker.org/docs/ref_builtins.html), `?string` is deprecated for certain type. Why isn't `${option.getValue()}` or `${option.getValue().toString()}` sufficient?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #655: DRILL-5047: When session option is string, query pr...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/655#discussion_r88547748
--- Diff: exec/java-exec/src/main/resources/rest/profile/profile.ftl ---
@@ -132,7 +132,7 @@
<#list model.getOptionList() as option>
<tr>
<td>${option.getName()}</td>
- <td>${option.getValue()?c}</td>
+ <td>${option.getValue()?string}</td>
--- End diff --
According to the [documentation](), ?string works only for numbers and formats them for "human display". ?c also works for numbers (and formats them as "C" language constants.)
The challenge is that this template line works for all data types. So, the suggestion of using toString( ) is good. But, since toString( ) may be used for debugging, perhaps add a toDisplayString( ) that will format the value as we want it to appear in the Web UI.
Another issue: are we sure that the value of option is always non-null at this point?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #655: DRILL-5047: When session option is string, query pr...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/655
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #655: DRILL-5047: When session option is string, query pr...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:
https://github.com/apache/drill/pull/655#discussion_r88514074
--- Diff: exec/java-exec/src/main/resources/rest/profile/profile.ftl ---
@@ -132,7 +132,7 @@
<#list model.getOptionList() as option>
<tr>
<td>${option.getName()}</td>
- <td>${option.getValue()?c}</td>
+ <td>${option.getValue()?string}</td>
--- End diff --
I am not familiar with FreeMarker so I do not understand the change. In Drill-4792 you mentioned
> Since org.apache.drill.exec.server.options.OptionValue.getValue() returns Object, Freemarker built-in c is used to convert Object to string.
Could you please explain why that was not sufficient and how using `string` changes that?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #655: DRILL-5047: When session option is string, query profile i...
Posted by Ben-Zvi <gi...@git.apache.org>.
Github user Ben-Zvi commented on the issue:
https://github.com/apache/drill/pull/655
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #655: DRILL-5047: When session option is string, query profile i...
Posted by kkhatua <gi...@git.apache.org>.
Github user kkhatua commented on the issue:
https://github.com/apache/drill/pull/655
@arina-ielchiieva Your fix will not conflict, but is in a branch rebased off 4b1902c .
@sudheeshkatkam had reverted the commit for DRILL-4373 2 days later. He is using the following branch to prepare for the Apache commit (including the invite to vote).
[Ref: https://github.com/sudheeshkatkam/drill/commits/drill-1.9.0 ]
So, you'll need to rebase it before making a pull request.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #655: DRILL-5047: When session option is string, query profile i...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on the issue:
https://github.com/apache/drill/pull/655
Thanks for the explanation and the corresponding changes. LGTM.
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #655: DRILL-5047: When session option is string, query profile i...
Posted by arina-ielchiieva <gi...@git.apache.org>.
Github user arina-ielchiieva commented on the issue:
https://github.com/apache/drill/pull/655
Replaced `HashMap` with `TreeMap` for sorted display.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---