[GitHub] drill issue #655: DRILL-5047: When session option is string, query profile i...

2016-11-18 Thread gparai
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---


[GitHub] drill issue #655: DRILL-5047: When session option is string, query profile i...

2016-11-18 Thread arina-ielchiieva
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---


[GitHub] drill issue #655: DRILL-5047: When session option is string, query profile i...

2016-11-18 Thread sudheeshkatkam
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---


[GitHub] drill issue #655: DRILL-5047: When session option is string, query profile i...

2016-11-18 Thread arina-ielchiieva
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`. 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---


[GitHub] drill issue #655: DRILL-5047: When session option is string, query profile i...

2016-11-17 Thread kkhatua
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---


[GitHub] drill issue #655: DRILL-5047: When session option is string, query profile i...

2016-11-17 Thread Ben-Zvi
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---