GH-35828: [Go] Add `array.WithUnorderedMapKeys` option for `array.ApproxEqual` #35823

candiduslynx · 2023-05-30T12:49:04Z

Rationale for this change

If we store the value of the *array.Map into Go map we can't expect to put the values back in the same order, as the map traversal order in Go is undefined.
This PR extends array.ApproxEqual by allowing map keys to be in different order.

What changes are included in this PR?

New helper functions:

array.arrayApproxEqualMap that is now used instead of array.arrayApproxEqualList for map comparison
array.arrayApproxEqualSingleMapEntry that checks if the single map entry we have matches to the other one without having keys sorted the same way

Are these changes tested?

Newly added test array.TestArrayApproxEqualMaps
fix(deps): Update github.com/cloudquery/plugin-sdk/v3 to v3.8.1 cloudquery/cloudquery#11078

Are there any user-facing changes?

New array.WithUnorderedMapKeys option for array.ApproxEqual that allows users to control if the entries order is important or not.

Closes: [Go] array.ApproxEqual should allow map entries be presented in any order #35828

github-actions · 2023-05-30T12:49:23Z

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 · 2023-05-30T13:59:35Z

@zeroshade it seems there's still a flaky test in FlightSQL

zeroshade · 2023-05-30T14:43:43Z

@candiduslynx yea, I'm aware. I've been periodically trying to track down the cause of the flakiness but haven't yet been able to do so. I'll kick the tires and re-run it, it should go away. Hopefully I (or someone) is able to track down the cause soon. It's quite annoying.

In the meantime, I'll take a look at this in a bit. Thanks!

raulcd · 2023-05-30T15:23:59Z

Can we create an issue for this one? It doesn't feel it matches the requirements for a MINOR one as seen here: https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#minor-fixes

candiduslynx · 2023-05-30T16:29:51Z

@zeroshade I've opened #35828

github-actions · 2023-05-30T16:29:54Z

⚠️ GitHub issue #35828 has been automatically assigned in GitHub to PR creator.

raulcd · 2023-05-30T16:55:29Z

@zeroshade I've opened #35828

Thank you!

go/arrow/array/compare.go

candiduslynx · 2023-05-30T17:56:02Z

@zeroshade I've moved the control over the new behavior into an option, so the users will not have any breaking changes.

candiduslynx · 2023-05-31T10:01:25Z

@zeroshade is there a chance to get it merged sooner? It's a blocker for cloudquery/cloudquery#11078

go/arrow/array/compare.go

zeroshade

looks good in general to me, just one question about the WithUnorderedMapKeys vs just checking the KeysSorted member of the MapType and deciding to go with the Unordered check if KeysSorted is false.

candiduslynx · 2023-05-31T15:30:00Z

looks good in general to me, just one question about the WithUnorderedMapKeys vs just checking the KeysSorted member of the MapType and deciding to go with the Unordered check if KeysSorted is false.

@zeroshade I've looked at the KeysSorted and it doesn't guarantee anything in the code (as well as the docstring for it states that this value isn't used in comparison in any way).

I believe updating the code to use KeysSorted is a breaking change & it should also imply the need to check for the entries order in the builder. So, I'd rather go with WithUnorderedMapKeys option for now & let someone who is really sure they need this functionality open an issue for this.

zeroshade

@candiduslynx that sounds good to me and works fine.

Blocked by apache/arrow#35823 Required by cloudquery/cloudquery#11078

ursabot · 2023-06-01T11:17:36Z

Benchmark runs are scheduled for baseline = fee8bf9 and contender = d9c2d59. d9c2d59 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.88% ⬆️0.18%] test-mac-arm
[Finished ⬇️1.3% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.27% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] d9c2d596 ec2-t3-xlarge-us-east-2
[Finished] d9c2d596 test-mac-arm
[Finished] d9c2d596 ursa-i9-9960x
[Finished] d9c2d596 ursa-thinkcentre-m75q
[Finished] fee8bf9c ec2-t3-xlarge-us-east-2
[Finished] fee8bf9c test-mac-arm
[Finished] fee8bf9c ursa-i9-9960x
[Finished] fee8bf9c ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

…11078) Extracted from #11043 Depends on: * cloudquery/plugin-sdk#913 * apache/arrow#35823 * cloudquery/plugin-sdk#921

add arrayApproxEqualMap

1f9a3d1

candiduslynx requested a review from zeroshade as a code owner May 30, 2023 12:49

github-actions bot added Component: Go awaiting review Awaiting review labels May 30, 2023

candiduslynx added a commit to cloudquery/cloudquery that referenced this pull request May 30, 2023

Use arr approx equal from apache/arrow#35823

8da29f9

release in arrayApproxEqualSingleMapEntry

14a5409

candiduslynx added a commit to cloudquery/cloudquery that referenced this pull request May 30, 2023

Use arr approx equal from apache/arrow#35823

2cf00aa

fix comment

9645ebe

candiduslynx added a commit to cloudquery/cloudquery that referenced this pull request May 30, 2023

Use arr approx equal from apache/arrow#35823

d5b7b01

candiduslynx mentioned this pull request May 30, 2023

fix(deps): Update github.com/cloudquery/plugin-sdk/v3 to v3.8.1 cloudquery/cloudquery#11078

Merged

candiduslynx changed the title ~~[Go] array.ApproxEqual allows map entries with different key order~~ MINOR: [Go] array.ApproxEqual allows map entries with different key order May 30, 2023

candiduslynx added 2 commits May 30, 2023 16:31

add test

196c7d8

go 1.17 fix

f5d2dbb

candiduslynx changed the title ~~MINOR: [Go] array.ApproxEqual allows map entries with different key order~~ GH-35828: [Go] array.ApproxEqual allows map entries with different key order May 30, 2023

zeroshade requested changes May 30, 2023

View reviewed changes

go/arrow/array/compare.go Outdated Show resolved Hide resolved

go/arrow/array/compare.go Outdated Show resolved Hide resolved

go/arrow/array/compare.go Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels May 30, 2023

move to an option

e66e55c

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 30, 2023

versatile test

0d82c06

candiduslynx requested a review from zeroshade May 30, 2023 17:51

candiduslynx changed the title ~~GH-35828: [Go] array.ApproxEqual allows map entries with different key order~~ GH-35828: [Go] Add array.WithUnorderedMapKeys option for array.ApproxEqual May 30, 2023

add diff value test, too

e288089

candiduslynx mentioned this pull request May 31, 2023

fix(test): Use array.WithUnorderedMapKeys cloudquery/plugin-sdk#921

Merged

zeroshade reviewed May 31, 2023

View reviewed changes

go/arrow/array/compare.go Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels May 31, 2023

zeroshade reviewed May 31, 2023

View reviewed changes

candiduslynx requested a review from zeroshade May 31, 2023 15:30

zeroshade approved these changes May 31, 2023

View reviewed changes

zeroshade merged commit d9c2d59 into apache:main May 31, 2023

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels May 31, 2023

candiduslynx deleted the feat/map-approx-equal branch May 31, 2023 17:52

kodiakhq bot pushed a commit to cloudquery/plugin-sdk that referenced this pull request Jun 1, 2023

fix(test): Use array.WithUnorderedMapKeys (#921)

ac2cfbd

Blocked by apache/arrow#35823 Required by cloudquery/cloudquery#11078

candiduslynx added a commit to cloudquery/cloudquery that referenced this pull request Jun 1, 2023

Use arr approx equal from apache/arrow#35823

a8d10cf

candiduslynx added a commit to cloudquery/cloudquery that referenced this pull request Jun 1, 2023

Use arr approx equal from apache/arrow#35823

8709696

GH-35828: [Go] Add array.WithUnorderedMapKeys option for array.ApproxEqual #35823

GH-35828: [Go] Add array.WithUnorderedMapKeys option for array.ApproxEqual #35823

Uh oh!

Conversation

candiduslynx commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented May 30, 2023

Uh oh!

candiduslynx commented May 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeroshade commented May 30, 2023

Uh oh!

raulcd commented May 30, 2023

Uh oh!

candiduslynx commented May 30, 2023

Uh oh!

github-actions bot commented May 30, 2023

Uh oh!

raulcd commented May 30, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

candiduslynx commented May 30, 2023

Uh oh!

candiduslynx commented May 31, 2023

Uh oh!

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

candiduslynx commented May 31, 2023

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

ursabot commented Jun 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GH-35828: [Go] Add `array.WithUnorderedMapKeys` option for `array.ApproxEqual` #35823

GH-35828: [Go] Add `array.WithUnorderedMapKeys` option for `array.ApproxEqual` #35823

candiduslynx commented May 30, 2023 •

edited

Loading

candiduslynx commented May 30, 2023 •

edited

Loading