Conversation
| // Special case: If string is empty and startIndex is 0, return empty string | ||
| if stringLen == 0 && startIndex == 0 { | ||
| return result.New("") | ||
| } |
There was a problem hiding this comment.
what should Substring('', 0) produce? The current code returns an empty string instead of Null (it seems reasonable to get the identical string when doing Substring(str, 0) == str to me). The spec would probably imply Null instead of empty string though. Could be worth clarifying/discussing, happy to change that back to the strict spec behavior.
There was a problem hiding this comment.
I think empty string is reasonable here.
I put up a change on the cqltests repo with this as a suggested new case cqframework/cql-tests#55
| wantResult: newOrFatal(t, -1), | ||
| }, | ||
| } | ||
| { |
There was a problem hiding this comment.
Note that these changes are because gofmt was not applied to the file on main. We should consider ensuring all files submitted to main are gofmt'd
There was a problem hiding this comment.
Yeah, agreed that we should add a check for this in the pipeline.
| wantResult: newOrFatal(t, nil), | ||
| }, | ||
| { | ||
| name: "Substring('😊😊😊', 1) -> 😊😊 (Unicode)", |
suyashkumar
force-pushed
the
cql-substring-operator
branch
from
8b11a13 to
f5bc7ef
Compare
This commit introduces the implementation for the CQL Substring operator. * Initial jules implementation * Correctly support startIndex=stringLen case, correct tests * Refactor for readability Author: Suyash Kumar <suyashkumar2003@gmail.com>
suyashkumar
force-pushed
the
cql-substring-operator
branch
from
f5bc7ef to
819f671
Compare
|
Hm I rebased and squashed the commit, not sure why the copybara import is failing. Any logs on your end? I could also try opening a new PR |
I reran the pipeline with debugging enabled and it looks there might be a real failure related to the rebase? PTAL. If it still looks wonky and tests pass locally then yeah we should just consider a different PR. |
|
Sounds good! The tests should all pass. Lmk if copybara still complains and I'll open a new PR. |
2 participants
This implements the CQL Substring operator.
One interesting note: what should
Substring('', 0)produce? The current code returns an empty string instead of Null (it seems reasonable to get the identical string when doingSubstring(str, 0) == strto me). The spec would probably imply Null. Could be worth clarifying/discussing, happy to change that back.Some code generated with the help of https://github.com/apps/google-labs-jules !