Support skip_values in DictionaryDecoder #2097
Conversation
| assert!(matches!(output, DictionaryBuffer::Dict { .. })); | ||
|
|
||
| assert_eq!(decoder.skip_values(1).unwrap(), 1); | ||
| assert_eq!(decoder.read(&mut output, 0..3).unwrap(), 3); |
There was a problem hiding this comment.
@tustvold Like you metion in #2076 (comment)
If i try to use assert_eq!(decoder.read(&mut output, 3..6).unwrap(), 3);
The result is not right, also appear in test_dictionary_preservation
i think we should align it.
There was a problem hiding this comment.
I think it should be 2..5 if anything, you've only read 2 values
| assert!(matches!(output, DictionaryBuffer::Dict { .. })); | ||
|
|
||
| assert_eq!(decoder.skip_values(1).unwrap(), 1); | ||
| assert_eq!(decoder.read(&mut output, 0..3).unwrap(), 3); |
There was a problem hiding this comment.
I think it should be 2..5 if anything, you've only read 2 values
| .as_ref() | ||
| .ok_or_else(|| general_err!("missing dictionary page for column"))?; | ||
|
|
||
| if dict.is_empty() { |
There was a problem hiding this comment.
I'm not sure why this check is necessary, removing the corresponding check from read, doesn't seem to fail any tests
| assert_eq!(decoder.skip_values(1).unwrap(), 1); | ||
| assert_eq!(decoder.read(&mut output, 0..3).unwrap(), 3); | ||
|
|
||
| valid.extend_from_slice(&[false, false, false, true, false, true, true, false]); |
There was a problem hiding this comment.
I think these tests would be easier to follow if they didn't include the null padding logic, which seems unnecessary for testing the skip behaviour
|
duplicate with #2100 |
Which issue does this PR close?
Closes #2709.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?