Turn jsonb stringify functions into separate functions#32055
Turn jsonb stringify functions into separate functions#32055antiguru merged 1 commit intoMaterializeInc:mainfrom
Conversation
ggevay
left a comment
ggevay
left a comment
There was a problem hiding this comment.
LGTM; the CI failures have straightforward fixes:
The lint is just the protobuf compatibility stuff, also discussed on slack.
The cargo test fail is due to some mzreflect-based tests. There are several occurrences of the functions that are being modified in the PR. For example
(call_binary (jsonb_get_string true) #0 ("created_ms" string))
has to be modified to
(call_binary (jsonb_get_string_stringify) #0 ("created_ms" string))
There are also some mzreflect-based tests that are not complaining, because it seems mzreflect just ignores the extra argument, but still should be changed. For example, there is a
(call_binary (jsonb_get_int64 false) #1 #0))
in src/expr/tests/testdata/reduce.
Normally, I'd debug why mzreflect is ignoring the extra argument, but mzreflect is on its way out, so we can just quickly fix the individual tests. I recommend grepping for jsonb_get_string true/jsonb_get_string false with all the modified functions to find all relevant tests.
The slt fail in test/sqllogictest/distinct_arrangements.slt is because some arrangement names mention JsonbGetString where this is part of the arrangement key in some builtin objects. This just needs a --rewrite-results.
| google.protobuf.Empty jsonb_get_string = 99; | ||
| google.protobuf.Empty jsonb_get_string_stringify = 198; | ||
| google.protobuf.Empty jsonb_get_path = 100; | ||
| google.protobuf.Empty jsonb_get_path_stringify = 199; |
There was a problem hiding this comment.
CI is complaining about compatibility, but as discussed on Slack, you could put an ignore into this file.
antiguru
force-pushed
the
jsonb_stringify
branch
from
190362f to
53af45b
Compare
|
Thanks for the review! |
Some of the jsonb functions can be used in contexts where the output is either jsonb or strings. Previously, we'd encode this as part of the function call by supplying a `bool` field. This is an abstraction leak and instead it would be better to have separate functions. This change absorbs the `stringify` parameter into different functions. No behavior change expected. Signed-off-by: Moritz Hoffmann <mh@materialize.com>
antiguru
force-pushed
the
jsonb_stringify
branch
from
53af45b to
1548366
Compare
2 participants
Some of the jsonb functions can be used in contexts where the output is either jsonb or strings. Previously, we'd encode this as part of the function call by supplying a
boolfield. This is an abstraction leak and instead it would be better to have separate functions.This change absorbs the
stringifyparameter into different functions. No behavior change expected.Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.