Skip to content

GH-51351: [C++][Parquet] Use default KMS connection values - #53996

Open
Dricmoy wants to merge 2 commits into
apache:mainfrom
Dricmoy:gh-51351-kms-default-config
Open

Dricmoy wants to merge 2 commits into
apache:mainfrom
Dricmoy:gh-51351-kms-default-config

Conversation

@Dricmoy

@Dricmoy Dricmoy commented Oct 16, 2026 •

Copy link
Copy Markdown

Rationale for this change

FileKeyWrapper calls SetDefaultIfEmpty() on its stored configuration, but passes the original, unmodified configuration to GetKmsClient. This allows the KMS client factory to receive empty instance ID and URL values even though Arrow has already filled in defaults on its internal copy.

What changes are included in this PR?

  • Pass the defaulted kms_connection_config_ member to GetKmsClient.
  • Update C++ and Python key-rotation tests to expect Arrow's existing default values for empty fields.
  • Separate the writer and reader crypto factories in the dataset encryption tests.

The dataset tests give the writer all keys and the reader only some. With a shared factory, the reader can reuse the writer's cached client, which already has all the keys. Separate factories ensure the reader uses its own keys, so the missing-key checks work correctly. The write-path fix exposes this because writing and reading now use the same default instance ID, where empty and "DEFAULT" IDs previously selected separate cache entries.

Are these changes tested?

  • Local C++ encryption key-management suite: 24 tests passed.
  • Both local Python encryption test files: 62 passed, 1 skipped. The skipped case requires a build without encryption support.
  • C++ and Python formatting and lint, and git diff --check, passed.
  • Fork CI for 537e296384: Python, Dev, and Docs passed. The C++ workflow passed for the preceding commit; the follow-up commit changes only Python tests.

Are there any user-facing changes?

KMS client factories receive Arrow's "DEFAULT" markers instead of empty strings for omitted instance ID and URL values. Explicit non-empty values are preserved. These markers are interpreted by the KMS implementation; they are not automatically resolved server addresses.

I reviewed the compatibility tradeoff: custom factories that special-case empty strings may need updating. Defaulting the instance ID can also change client reuse because the cache is indexed by access token and instance ID. Requests with the same token and resolved ID can now share a client where empty and "DEFAULT" IDs previously selected separate entries. This makes client creation consistent with Arrow's existing defaulting logic and stored key metadata, but changes behavior for integrations that rely on empty strings. There are no API signature changes.

Closes #51351. Follow-up to #53949.

Was AI used for this PR?

AI assisted with the implementation, tests, validation, and this description. I reviewed the configuration flow, the default-value test expectations, the reader/writer cache separation, and the compatibility tradeoff described above. The PR overall is fairly small consistent expectation change, had some tests changed to fit the updated expectations.

PR code and description written by:

  • Human
  • AI

Reviewed before submission by:

  • Human
  • AI
  • Not reviewed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++][Parquet] FileKeyWrapper uses KMSConnectionConfig without setting default values

1 participant