Skip to content

test(csharp): compare MAP columns order-insensitively in E2E tests - #662

Merged
eric-wang-1990 merged 2 commits into
mainfrom
eric-wang/csharp-map-tests-order-insensitive
Sep 4, 2026
Merged

test(csharp): compare MAP columns order-insensitively in E2E tests#662
eric-wang-1990 merged 2 commits into
mainfrom
eric-wang/csharp-map-tests-order-insensitive

Conversation

@eric-wang-1990

Copy link
Copy Markdown
Collaborator

Problem

ComplexTypesValueTests.TestMapData and ClientTests.VerifyTypesAndValues compare a serialized MAP column by exact JSON string, which bakes in a specific key order. But Databricks MAP key order is unspecified — the MAP type is an unordered set, and the server demonstrably returns keys in different orders on different runs.

This surfaced in #509's merge queue: on the identical commit, E2E (thrift + rest) passed at 17:39 (server returned sorted keys) and failed at ~22:00 (server returned insertion order):

Expected: {"Jack Doe":3,"Jane Doe":2,"John Doe":1}
Actual:   {"John Doe":1,"Jane Doe":2,"Jack Doe":3}

#509 removed the client-side SortedDictionary in ComplexTypeSerializingStream that used to normalize map output to sorted order (on both Thrift and SEA — both wrap with that serializer). That normalization was masking the fact that these tests asserted an order the server never guaranteed. With it gone, the tests flake on both protocols.

Why not just re-sort in the driver?

Because the driver shouldn't. JDBC — the reference contract this driver matches — does not sort map keys; it preserves the server's order via LinkedHashMap. The old SortedDictionary was an ADBC-only divergence from JDBC (and a string sort, so it mis-ordered numeric keys as 1,10,2). #509's no-sort behavior is the more JDBC-conformant, spec-faithful choice. The tests are what needed fixing.

Fix

Assert map content, not an unspecified key order:

  • Add DatabricksTestEnvironment.NormalizeMapJson — sorts the top-level object's keys (Ordinal), preserving each value's raw JSON so nested STRUCT field order (which is significant) is untouched.
  • TestMapData override and the VerifyTypesAndValues map sample column now compare NormalizeMapJson(expected) vs NormalizeMapJson(actual) (the latter via the existing isValid validator hook).

No driver/serializer changes; no submodule or upstream arrow-adbc changes.

Verification

  • dotnet build of the test project succeeds.
  • Normalization is applied to both sides identically, so the assertion is invariant to the server's key order — fixing the flake on both Thrift and SEA. (Cannot run the E2E suite locally — needs a live warehouse; relies on CI, which is where the failure reproduced.)

Unblocks #509's merge queue: order-insensitive comparison passes on both the old (sorted) and #509 (server-order) serializer output, so this can land on main independently, after which #509 rebases and passes.

This pull request and its description were written by Isaac.

Databricks MAP key order is unspecified — the server may return keys in any
order (the MAP type is an unordered set), and it demonstrably does: the same
commit produced sorted keys in one CI run and insertion order in another. JDBC
preserves the server's order via LinkedHashMap without sorting, and #509
(adbc-drivers/databricks) removed the client-side SortedDictionary that used to
normalize SEA/Thrift map output to sorted. With that normalization gone,
ComplexTypesValueTests.TestMapData and ClientTests.VerifyTypesAndValues — which
compared the serialized map JSON by exact string — flake on both protocols
whenever the server returns a non-sorted order.

Fix the tests to assert map CONTENT, not an unspecified key order: add
DatabricksTestEnvironment.NormalizeMapJson (sorts the top-level object's keys,
preserving each value's raw JSON so nested STRUCT field order is untouched) and
compare normalized expected vs actual in both tests. The driver keeps #509's
no-sort behavior (matches JDBC and the unordered MAP semantics).

Co-authored-by: Isaac
@eric-wang-1990 eric-wang-1990 added the engineer-bot engineer-bot may fix this issue / take over this PR label Sep 2, 2026

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks good — a focused test-only fix that compares MAP columns by content rather than an unspecified server key order. NormalizeMapJson, the ColumnNetTypeArrowTypeValue(..., bool, isValid) overload it uses, and the overridden ValidateTestMapData all line up correctly; normalization is applied identically to both sides so the assertion is order-invariant. One low note about the normalization being intentionally shallow (top-level keys only).

Comment thread csharp/test/E2E/DatabricksTestEnvironment.cs
Addresses:
  - #3919678255 at csharp/test/E2E/DatabricksTestEnvironment.cs:71

Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No issues identified by the review bot.

@eric-wang-1990
eric-wang-1990 added this pull request to the merge queue Sep 4, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 4, 2026
@eric-wang-1990
eric-wang-1990 added this pull request to the merge queue Sep 4, 2026
Merged via the queue into main with commit dbc82c6 Sep 4, 2026
25 checks passed
@eric-wang-1990
eric-wang-1990 deleted the eric-wang/csharp-map-tests-order-insensitive branch September 4, 2026 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engineer-bot engineer-bot may fix this issue / take over this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant