Skip to content

MINOR [Go] Add nested struct test for pqarrow#35851

Closed
candiduslynx wants to merge 2 commits intoapache:mainfrom
cloudquery:repro/parquet-nested-struct-null
Closed

MINOR [Go] Add nested struct test for pqarrow#35851
candiduslynx wants to merge 2 commits intoapache:mainfrom
cloudquery:repro/parquet-nested-struct-null

Conversation

@candiduslynx
Copy link
Copy Markdown
Contributor

@candiduslynx candiduslynx commented May 31, 2023

@zeroshade I've found a disturbing behavior with structs (namely, nested ones) in pqarrow.

This blocks cloudquery/filetypes#172

Could you please take a look?

@candiduslynx candiduslynx requested a review from zeroshade as a code owner May 31, 2023 17:08
@github-actions
Copy link
Copy Markdown

github-actions bot commented May 31, 2023

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@candiduslynx candiduslynx mentioned this pull request May 31, 2023
Merged
zeroshade
zeroshade reviewed May 31, 2023
View reviewed changes
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels May 31, 2023
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 31, 2023
@candiduslynx
Copy link
Copy Markdown
Contributor Author

candiduslynx commented May 31, 2023

@zeroshade I found an issue with the test & it should now pass, but I'm really confused about array.NewStructData behavior in conjunction with pqarrow.
Could you please take a look at the code in cloudquery/filetypes#172?
Specifically, the issue I see is that the offsets/null bitmaps for the nested field are propagated strangely.
It panics while trying to create Struct array from data, so I don't know where to look.

@candiduslynx candiduslynx requested a review from zeroshade May 31, 2023 17:52
@candiduslynx candiduslynx changed the title [Go] Repro for nested null struct panic MINOR [Go] Add nested struct test for pqarrow May 31, 2023
@candiduslynx
Copy link
Copy Markdown
Contributor Author

candiduslynx commented May 31, 2023

I guess the issue can be connected to https://github.com/apache/arrow/blob/main/go/parquet/pqarrow/encode_arrow_test.go#L1040-L1042

// current impl of ArrayEquals for structs doesn't correctly handle nulls in the parent
// with a non-nullable child when comparing. Since after the round trip, the data in the
// child will have the nulls, not the original data.

kodiakhq bot pushed a commit to cloudquery/filetypes that referenced this pull request Jun 1, 2023
@candiduslynx
fix(deps): Update module github.com/cloudquery/plugin-sdk/v3 to v3.8.1 (
01c789b 
#172)

Extracted from #171

Structs reading is slow because of apache/arrow#35851
Fields copy is because of apache/arrow#35867
@candiduslynx
Copy link
Copy Markdown
Contributor Author

candiduslynx commented Jun 1, 2023

I'm closing this for now, however, it would've been nice to be able to use array.NewStructData on the read values

@candiduslynx candiduslynx mentioned this pull request Jun 1, 2023
Merged
zeroshade pushed a commit that referenced this pull request Jun 1, 2023
@candiduslynx
GH-35871: [Go] Account for struct validity bitmap in `array.ApproxEqu…
61692b6 
…al` (#35872)

### Rationale for this change

When comparing `array.Struct` values with `array.ApproxEqual` the validity bitmap of the struct itself should take precedence:
> When reading the struct array the parent validity bitmap takes priority.

This follows a brief discussion from #35851.

### What changes are included in this PR?

`array.arrayApproxEqualStruct` will check the fields data only if the struct elem is valid.

### Are these changes tested?

`pqarrow` tests are updated accordingly (no special treatment for structs, just `array.ApproxEqual`

### Are there any user-facing changes?

`array.ApproxEqual` behavior changed to match the docs about validity bitmap precedence.

* Closes: #35871

Authored-by: candiduslynx <candiduslynx@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zeroshade zeroshade Awaiting requested review from zeroshade

Assignees

No one assigned

Labels

awaiting change review Awaiting change review Component: Go

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@candiduslynx @zeroshade