You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "John Russell (Code Review)" <ge...@cloudera.org> on 2017/01/03 18:42:53 UTC
[Impala-ASF-CR] IMPALA-3973: optional 2nd and 3rd arguments for instr().
John Russell has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5589
Change subject: IMPALA-3973: optional 2nd and 3rd arguments for instr().
......................................................................
IMPALA-3973: optional 2nd and 3rd arguments for instr().
Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
---
M docs/topics/impala_string_functions.xml
1 file changed, 79 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/5589/1
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
[Impala-ASF-CR] IMPALA-3973: optional 3rd and 4th arguments for instr().
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3973: optional 3rd and 4th arguments for instr().
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5589/2/docs/topics/impala_string_functions.xml
File docs/topics/impala_string_functions.xml:
PS2, Line 335: 1
> Done
What line? I don't see it.
Line 406: select instr('hello world','o',-1);
> Done
What line is the negative position argument and an occurence argument on? 444 and 447 are invalid input.
http://gerrit.cloudera.org:8080/#/c/5589/4/docs/topics/impala_string_functions.xml
File docs/topics/impala_string_functions.xml:
PS4, Line 352: +------------------------------------+
not exactly: that position does not exist.
PS4, Line 415: 'o',
fourth
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3973: optional 3rd and 4th arguments for instr().
Posted by "John Russell (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Ivanfi,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5589
to look at the new patch set (#3).
Change subject: IMPALA-3973: optional 3rd and 4th arguments for instr().
......................................................................
IMPALA-3973: optional 3rd and 4th arguments for instr().
Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
---
M docs/topics/impala_string_functions.xml
1 file changed, 162 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/5589/3
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
[Impala-ASF-CR] IMPALA-3973: optional 2nd and 3rd arguments for instr().
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3973: optional 2nd and 3rd arguments for instr().
......................................................................
Patch Set 2:
> > Since I got the +1 from Zoltan, and I'd classify the subsequent
> > suggestions as nice-to-have but not essential, any objection to
> my
> > +2'ing it now and revisiting improvements later?
>
> Yes. The way things work on the code side is that all comments are
> addressed before submitting.
TO be even more specific: on the code side, all comments are addressed in the opinion of reviewer, not the patch author.
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3973: optional 3rd and 4th arguments for instr().
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-3973: optional 3rd and 4th arguments for instr().
......................................................................
Patch Set 6: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3973: optional 2nd and 3rd arguments for instr().
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3973: optional 2nd and 3rd arguments for instr().
......................................................................
Patch Set 2:
> (1 comment)
Any updates?
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3973: optional 3rd and 4th arguments for instr().
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: IMPALA-3973: optional 3rd and 4th arguments for instr().
......................................................................
Patch Set 5:
John, any progress on another patch set to address Jim's comments for lines "335 and 406"?
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3973: optional 3rd and 4th arguments for instr().
Posted by "John Russell (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Ivanfi,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5589
to look at the new patch set (#6).
Change subject: IMPALA-3973: optional 3rd and 4th arguments for instr().
......................................................................
IMPALA-3973: optional 3rd and 4th arguments for instr().
Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
---
M docs/topics/impala_string_functions.xml
1 file changed, 174 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/5589/6
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
[Impala-ASF-CR] IMPALA-3973: optional 2nd and 3rd arguments for instr().
Posted by "John Russell (Code Review)" <ge...@cloudera.org>.
John Russell has uploaded a new patch set (#2).
Change subject: IMPALA-3973: optional 2nd and 3rd arguments for instr().
......................................................................
IMPALA-3973: optional 2nd and 3rd arguments for instr().
Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
---
M docs/topics/impala_string_functions.xml
1 file changed, 113 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/5589/2
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
[Impala-ASF-CR] IMPALA-3973: optional 3rd and 4th arguments for instr().
Posted by "John Russell (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Ivanfi,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5589
to look at the new patch set (#4).
Change subject: IMPALA-3973: optional 3rd and 4th arguments for instr().
......................................................................
IMPALA-3973: optional 3rd and 4th arguments for instr().
Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
---
M docs/topics/impala_string_functions.xml
1 file changed, 165 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/5589/4
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
[Impala-ASF-CR] IMPALA-3973: optional 3rd and 4th arguments for instr().
Posted by "John Russell (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Ivanfi,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5589
to look at the new patch set (#5).
Change subject: IMPALA-3973: optional 3rd and 4th arguments for instr().
......................................................................
IMPALA-3973: optional 3rd and 4th arguments for instr().
Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
---
M docs/topics/impala_string_functions.xml
1 file changed, 165 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/5589/5
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
[Impala-ASF-CR] IMPALA-3973: optional 3rd and 4th arguments for instr().
Posted by "John Russell (Code Review)" <ge...@cloudera.org>.
John Russell has posted comments on this change.
Change subject: IMPALA-3973: optional 3rd and 4th arguments for instr().
......................................................................
Patch Set 2:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/5589/2/docs/topics/impala_string_functions.xml
File docs/topics/impala_string_functions.xml:
PS2, Line 335: 1
> Same as the "off the end of the string" possibility below. The 'occurrence'
Done
PS2, Line 335: 1
> Can the occurrence argument be 0?
Done
PS2, Line 335: 1
> Can you add a note to say that in this doc?
Done
PS2, Line 349: 7
> What if occurrence is off the end of the string?
Done
PS2, Line 349: 7
> That one's covered by
Done
PS2, Line 395: negative position argument
> That particular circumstance causes an error instead of a zero return value
Done
PS2, Line 395: negative position argument
> Can the occurrence argument be zero or negative?
Done
PS2, Line 395: negative position argument
> That's intentional. In fact, initially I returned 0 in this case as well, b
OK. The "UDF error" string in the error message gave me a different impression than when I see other errors from built-in functions. Usually the messages downplay the fact that built-in functions are just UDFs behind the scenes.
PS2, Line 395: negative position argument
> Can the behavior be explicitly called out in this patch's contents?
Done
Line 406: select instr('hello world','o',-1);
> I do not see a patch set 3 here yet. Are you planning on sending that in a
Done
Line 406: select instr('hello world','o',-1);
> Sure. In general, we try to be more detailed than Hive when it comes to pro
Done
Line 406: select instr('hello world','o',-1);
> Can you show an example with a negative position argument and an occurrence
Done
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3973: optional 2nd and 3rd arguments for instr().
Posted by "John Russell (Code Review)" <ge...@cloudera.org>.
John Russell has posted comments on this change.
Change subject: IMPALA-3973: optional 2nd and 3rd arguments for instr().
......................................................................
Patch Set 2:
I'll come back to this one shortly. Just going to pursue +2s in a couple of more knotty gerrits first.
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3973: optional 2nd and 3rd arguments for instr().
Posted by "Zoltan Ivanfi (Code Review)" <ge...@cloudera.org>.
Zoltan Ivanfi has posted comments on this change.
Change subject: IMPALA-3973: optional 2nd and 3rd arguments for instr().
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5589/2/docs/topics/impala_string_functions.xml
File docs/topics/impala_string_functions.xml:
PS2, Line 395: negative position argument
> That particular circumstance causes an error instead of a zero return value
That's intentional. In fact, initially I returned 0 in this case as well, but then acting on Dan Hecht's recommendation I checked this behavior in Oracle and found that Oracle gives an error in this case, so we do the same. Related discussion can be found on https://gerrit.cloudera.org/#/c/4094/13/be/src/exprs/string-functions-ir.cc@292
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3973: optional 2nd and 3rd arguments for instr().
Posted by "John Russell (Code Review)" <ge...@cloudera.org>.
John Russell has posted comments on this change.
Change subject: IMPALA-3973: optional 2nd and 3rd arguments for instr().
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5589/2/docs/topics/impala_string_functions.xml
File docs/topics/impala_string_functions.xml:
Line 406: select instr('hello world','o',-1);
> I do not see a patch set 3 here yet. Are you planning on sending that in a
Sorry, I had patch set 3 worked up over the weekend but it got borked by a corrupted hard drive before I could push. I've got some other CRs to attend to before I come back to this one.
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3973: optional 3rd and 4th arguments for instr().
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3973: optional 3rd and 4th arguments for instr().
......................................................................
Patch Set 5:
Any updates on this? The comment about line 335 was left on January 3rd.
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3973: optional 2nd and 3rd arguments for instr().
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3973: optional 2nd and 3rd arguments for instr().
......................................................................
Patch Set 2:
PS2 was posted more than two months ago. Are you interested in continuing this? If not, you can "Abandon" it by clicking "Abandon". You can un-Abandon later by clicking one button if you change your mind.
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3973: optional 2nd and 3rd arguments for instr().
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3973: optional 2nd and 3rd arguments for instr().
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5589/2/docs/topics/impala_string_functions.xml
File docs/topics/impala_string_functions.xml:
PS2, Line 335: 1
> Same as the "off the end of the string" possibility below. The 'occurrence'
Can you add a note to say that in this doc?
PS2, Line 395: negative position argument
> That particular circumstance causes an error instead of a zero return value
Can the behavior be explicitly called out in this patch's contents?
Line 406: select instr('hello world','o',-1);
> Sure. In general, we try to be more detailed than Hive when it comes to pro
I do not see a patch set 3 here yet. Are you planning on sending that in a later patch, or just haven't gotten to it yet?
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3973: optional 2nd and 3rd arguments for instr().
Posted by "John Russell (Code Review)" <ge...@cloudera.org>.
John Russell has posted comments on this change.
Change subject: IMPALA-3973: optional 2nd and 3rd arguments for instr().
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5589/2/docs/topics/impala_string_functions.xml
File docs/topics/impala_string_functions.xml:
PS2, Line 335: 1
> Can the occurrence argument be 0?
Same as the "off the end of the string" possibility below. The 'occurrence' arg is 1-based like the 'position' arg, and anything out of range produces a 0 result.
PS2, Line 349: 7
> What if occurrence is off the end of the string?
That one's covered by
select instr('foo bar bletch', 'b', 20, 1);
below. I grouped all the zero-returning examples together because there are several kinds of nonsensical argument combinations. Why don't I clarify that theme of out-of-range arguments in the intro text on line 365.
PS2, Line 395: negative position argument
> Can the occurrence argument be zero or negative?
That particular circumstance causes an error instead of a zero return value:
select instr('foo','f',1,-1);
ERROR: UDF ERROR: Invalid occurrence parameter to instr function: -1
I'm not sure if that's intentional or a exception that was intended to be caught but wasn't. (How do I @-mention Zoltan in a gerrit comment?)
Line 406: select instr('hello world','o',-1);
> Can you show an example with a negative position argument and an occurrence
Sure. In general, we try to be more detailed than Hive when it comes to providing examples of built-in functions, and to be more on par with docs for mainstream DBs like MySQL.
Hive: https://cwiki.apache.org/confluence/display/Hive/LanguageManual+UDF#LanguageManualUDF-StringFunctions
MySQL: http://dev.mysql.com/doc/refman/5.7/en/string-functions.html#function_instr
Those counterparts often don't specify behavior for all cases of argument ranges for string or math functions. But instr() is so commonly used and many counterparts don't have position/occurrence for instr(), I agree that additional examples are worthwhile.
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3973: optional 2nd and 3rd arguments for instr().
Posted by "John Russell (Code Review)" <ge...@cloudera.org>.
John Russell has posted comments on this change.
Change subject: IMPALA-3973: optional 2nd and 3rd arguments for instr().
......................................................................
Patch Set 2:
Since I got the +1 from Zoltan, and I'd classify the subsequent suggestions as nice-to-have but not essential, any objection to my +2'ing it now and revisiting improvements later?
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3973: optional 2nd and 3rd arguments for instr().
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3973: optional 2nd and 3rd arguments for instr().
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5589/2/docs/topics/impala_string_functions.xml
File docs/topics/impala_string_functions.xml:
PS2, Line 335: 1
Can the occurrence argument be 0?
PS2, Line 349: 7
What if occurrence is off the end of the string?
instr('a', 'a', 2)
PS2, Line 395: negative position argument
Can the occurrence argument be zero or negative?
Line 406: select instr('hello world','o',-1);
Can you show an example with a negative position argument and an occurrence argument?
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward #268
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3973: optional 3rd and 4th arguments for instr().
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-3973: optional 3rd and 4th arguments for instr().
......................................................................
IMPALA-3973: optional 3rd and 4th arguments for instr().
Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Reviewed-on: http://gerrit.cloudera.org:8080/5589
Reviewed-by: Jim Apple <jb...@apache.org>
Tested-by: Impala Public Jenkins
---
M docs/topics/impala_string_functions.xml
1 file changed, 173 insertions(+), 1 deletion(-)
Approvals:
Impala Public Jenkins: Verified
Jim Apple: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
[Impala-ASF-CR] IMPALA-3973: optional 3rd and 4th arguments for instr().
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3973: optional 3rd and 4th arguments for instr().
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3973: optional 3rd and 4th arguments for instr().
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-3973: optional 3rd and 4th arguments for instr().
......................................................................
Patch Set 6:
Build started: http://jenkins.impala.io:8080/job/gerrit-docs-submit/114/
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3973: optional 2nd and 3rd arguments for instr().
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Anonymous Coward #268 has posted comments on this change.
Change subject: IMPALA-3973: optional 2nd and 3rd arguments for instr().
......................................................................
Patch Set 2: Code-Review+1
Thanks for the doc update!
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward #268
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3973: optional 2nd and 3rd arguments for instr().
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-3973: optional 2nd and 3rd arguments for instr().
......................................................................
Patch Set 2:
> Since I got the +1 from Zoltan, and I'd classify the subsequent
> suggestions as nice-to-have but not essential, any objection to my
> +2'ing it now and revisiting improvements later?
Yes. The way things work on the code side is that all comments are addressed before submitting.
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3973: optional 3rd and 4th arguments for instr().
Posted by "John Russell (Code Review)" <ge...@cloudera.org>.
John Russell has posted comments on this change.
Change subject: IMPALA-3973: optional 3rd and 4th arguments for instr().
......................................................................
Patch Set 2:
(2 comments)
Added one example that had gotten lost from previous patch set. Clarified one other example that used 2 negative arguments; wasn't clear which negative arg was causing the behavior shown in the example.
http://gerrit.cloudera.org:8080/#/c/5589/2/docs/topics/impala_string_functions.xml
File docs/topics/impala_string_functions.xml:
PS2, Line 335: 1
> What line? I don't see it.
From patch set 5 (note "or zero" text and first example with final param 0):
The fourth argument cannot be negative or zero. A non-positive value for this argument causes an error:
select instr('foo bar bletch', 'b', 1, 0);
ERROR: UDF ERROR: Invalid occurrence parameter to instr function: 0
select instr('aaaaaaaaa','aa',-1,-1);
ERROR: UDF ERROR: Invalid occurrence parameter to instr function: -1
I was mistaken in original reply thinking that the 0 would be accepted and give a 0 return value.
I will change the -1,-1 final 2 args of the second example above to 1,-1 to make it clear which negative arg is causing the error.
Line 406: select instr('hello world','o',-1);
> What line is the negative position argument and an occurence argument on? 4
Line 413 of patch set 6. I included it in the examples for the occurrence argument because otherwise the meaning of that final argument hadn't been explained.
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3973: optional 3rd and 4th arguments for instr().
Posted by "John Russell (Code Review)" <ge...@cloudera.org>.
John Russell has posted comments on this change.
Change subject: IMPALA-3973: optional 3rd and 4th arguments for instr().
......................................................................
Patch Set 5:
(2 comments)
Patch set 5 specifically for Jim's last 2 'PS4' comments. The line 335 and 406 ones I'll look at in a separate patch set.
http://gerrit.cloudera.org:8080/#/c/5589/4/docs/topics/impala_string_functions.xml
File docs/topics/impala_string_functions.xml:
PS4, Line 352: -- specified position, the result is 0.
> not exactly: that position does not exist.
Sure, I'll lower the number so it's a valid position within the string that still returns a 0 result.
PS4, Line 415: fourt
> fourth
Done
--
To view, visit http://gerrit.cloudera.org:8080/5589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Zoltan Ivanfi <zi...@cloudera.com>
Gerrit-Reviewer: zi+zicl@cloudera.com
Gerrit-HasComments: Yes