Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/query: Check individual mapping fields for null #3786

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

metalmatze
Copy link
Member

It seems like mappingStart was valid (not null) but the build ID index was.

@alwaysmeticulous
Copy link

alwaysmeticulous bot commented Sep 8, 2023

✅ Meticulous spotted zero visual differences across 406 screens tested: view results.

Last updated for commit 4043468. This comment will update as new commits are pushed.

@brancz
Copy link
Member

brancz commented Sep 8, 2023

Can we try to understand why this happens? As far as I can tell all codepaths that generate these inputs write a non-null value for all mapping fields if writing a mapping start, so I don't understand why this happens in the first place.

@brancz
Copy link
Member

brancz commented Sep 8, 2023

As in, this is the code that produces this:

parca/pkg/parcacol/arrow.go

Lines 338 to 354 in 00471e8

if loc.Mapping != nil {
w.MappingStart.Append(loc.Mapping.Start)
w.MappingLimit.Append(loc.Mapping.Limit)
w.MappingOffset.Append(loc.Mapping.Offset)
if err := w.MappingFile.Append([]byte(loc.Mapping.File)); err != nil {
return nil, fmt.Errorf("append mapping file: %w", err)
}
if err := w.MappingBuildID.Append([]byte(loc.Mapping.BuildId)); err != nil {
return nil, fmt.Errorf("append mapping build id: %w", err)
}
} else {
w.MappingStart.AppendNull()
w.MappingLimit.AppendNull()
w.MappingOffset.AppendNull()
w.MappingFile.AppendNull()
w.MappingBuildID.AppendNull()
}

@metalmatze
Copy link
Member Author

Really odd indeed. Reading around the code you posted I briefly assumed that we might be hitting this part:
https://github.com/apache/arrow/blob/50015f084846faaa5bf867346d045499e3f68316/go/arrow/array/dictionary.go#L1300-L1303

However, we are calling Append([]byte(loc.Mapping.BuildId)). loc.Mapping cannot be nil as we check that right above and even if loc.Mapping.BuildId == "" the dictionary won't appendNull, as "" != nil. 🤔

@brancz
Copy link
Member

brancz commented Sep 8, 2023

Is there maybe something special about the conversion of an empty string?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants