[Windowing] Handle null values in partitions #141
Conversation
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #141 +/- ##
==========================================
- Coverage 45.82% 45.78% -0.04%
==========================================
Files 47 47
Lines 17800 17804 +4
==========================================
- Hits 8156 8152 -4
- Misses 8182 8193 +11
+ Partials 1462 1459 -3 |
| @@ -274,6 +274,9 @@ type WindowFuncStatus struct { | |||
| OrderBy []*WindowOrderBy | |||
| } | |||
|
|
|||
| // windowNilPartitionValue Placeholder value for nil | |||
| const windowNilPartitionValue = StringValue("^^^ZETASQLITE_NIL^^^") | |||
There was a problem hiding this comment.
@goccy While exploring other issues, it seems this case comes up often.
Does it make sense to introduce a new NilValue class instead?
There was a problem hiding this comment.
NULL is not the first class's value. So I want to deal with this by handling null properly.
| @@ -274,6 +274,9 @@ type WindowFuncStatus struct { | |||
| OrderBy []*WindowOrderBy | |||
| } | |||
|
|
|||
| // windowNilPartitionValue Placeholder value for nil | |||
| const windowNilPartitionValue = StringValue("^^^ZETASQLITE_NIL^^^") | |||
There was a problem hiding this comment.
NULL is not the first class's value. So I want to deal with this by handling null properly.
| if v.Value != nil { | ||
| opt.Partitions = append(opt.Partitions, v.Value.(Value)) | ||
| } else { | ||
| opt.Partitions = append(opt.Partitions, windowNilPartitionValue) |
There was a problem hiding this comment.
Can't we just use nil instead of windowNilPartitionValue ?
There was a problem hiding this comment.
What would strings.Join("_", []string{"a", nil, "c"}) evaluate to?
The intention is that windowed rows that share common nil columns are partitioned together.
WITH toks as (
SELECT 'a' as prop1, null as prop2, 'aa' as prop3
UNION ALL SELECT 'a' as prop1, 'aa' as prop2, null as prop3
)
SELECT prop1, ROW_NUMBER() over (PARTITION BY prop2, prop3) AS rn FROM toksExpected:
| prop1 | rn |
|---|---|
| a | 1 |
| a | 1 |
There was a problem hiding this comment.
A guard expression against nil must be inserted before the join process.
func (s *WindowFuncStatus) Partition() (string, error) {
partitions := make([]string, 0, len(s.Partitions))
for _, p := range s.Partitions {
if p == nil {
continue
}
text, err := p.ToString()
if err != nil {
return "", err
}
partitions = append(partitions, text)
}
return strings.Join(partitions, "_"), nil
}
There was a problem hiding this comment.
In the query above, wouldn't that result in the following?
| prop1 | rn |
|---|---|
| a | 1 |
| a | 2 |
There was a problem hiding this comment.
Ah, I see. Then it looks like we should add constant NullValue structure.
Example.
- value.go
type NullValue struct {
zeroValue Value
}NullValue should have the original type information, so it should either have a zero value or reflect.Type .
If NULL value is integer type, initialize as follows
&NullValue{zeroValue: IntValue(0)}There was a problem hiding this comment.
I will try to do this as I think it is the right approach, too.
There was a problem hiding this comment.
Thank you !! Generics seems to be a better choice. 😃
type NullValue[T Value] struct {
zeroValue T
}
|
Closing in favor of #169! |
This inserts a placeholder value to use for partitioning when rows contain a null value.
Closes #139 #86