[GitHub] drill issue #1026: DRILL-5919: Add non-numeric support for JSON processing

2017-12-20 Thread arina-ielchiieva 
Github user arina-ielchiieva commented on the issue:

https://github.com/apache/drill/pull/1026
  
@vladimirtkach thanks for adding tests for math functions. New changes look 
good to me.
@paul-rogers please take a look.


---

[GitHub] drill issue #1026: DRILL-5919: Add non-numeric support for JSON processing

2017-12-08 Thread vladimirtkach 
Github user vladimirtkach commented on the issue:

https://github.com/apache/drill/pull/1026
  
@paul-rogers Made changes, please review. 


---

[GitHub] drill issue #1026: DRILL-5919: Add non-numeric support for JSON processing

2017-11-13 Thread paul-rogers 
Github user paul-rogers commented on the issue:

https://github.com/apache/drill/pull/1026
  
On the two functions... Maybe just have one function that handles the 
Nan/Infinity case. As noted earlier, no matter what we do, JSON without these 
symbols will work. So, we need only consider JSON with the symbols. Either:

* The NaN/Infinity cases always work, or
* The NaN/Infinity cases work sometimes, fail others, depending on some 
option or argument.

I would vote for the first case: it is simpler. I just can't see how anyone 
would use Drill to valid JSON and would want a query to fail if it contains NaN 
or Infinity. Can you suggest a case where failing the query would be of help to 
the user?


---

[GitHub] drill issue #1026: DRILL-5919: Add non-numeric support for JSON processing

2017-11-13 Thread vladimirtkach 
Github user vladimirtkach commented on the issue:

https://github.com/apache/drill/pull/1026
  
what do you think about having two functions instead: convertFromJSON and 
convertFromJSON+some suffix. Second will be able to convert  NaN, Infinity


---

[GitHub] drill issue #1026: DRILL-5919: Add non-numeric support for JSON processing

2017-11-10 Thread paul-rogers 
Github user paul-rogers commented on the issue:

https://github.com/apache/drill/pull/1026
  
Further, is the extra option to `convertFromJSON` really needed? Can't we 
just accept `NaN` and `Infinity` by default?

Consider. If the option is off by default, users without `NaN` or 
`Infinity` data will see no difference. But, users will this data will get an 
error and have to hunt down the option to make their data work.

If the option is on by default, users without `NaN` or `Infinity` data will 
see no difference. But, users will this data will also have their queries work 
by default.

So, seems no harm in making the `NaN` and `Infinity` support turned on by 
default.


---

[GitHub] drill issue #1026: DRILL-5919: Add non-numeric support for JSON processing

2017-11-09 Thread arina-ielchiieva 
Github user arina-ielchiieva commented on the issue:

https://github.com/apache/drill/pull/1026
  
Thanks, +1, LGTM.


---

[GitHub] drill issue #1026: DRILL-5919: Add non-numeric support for JSON processing

2017-11-09 Thread vladimirtkach 
Github user vladimirtkach commented on the issue:

https://github.com/apache/drill/pull/1026
  
@arina-ielchiieva made code changes according to your comments.


---