dwsmith1983 commented on code in PR #5365:
URL: https://github.com/apache/datafusion-comet/pull/5365#discussion_r4157738644
##########
native/core/src/parquet/objectstore/s3.rs:
##########
@@ -1002,10 +1037,97 @@ impl CredentialProviderMetadata {
#[cfg(test)]
mod tests {
+ use std::collections::BTreeSet;
use std::sync::atomic::{AtomicI32, Ordering};
use super::*;
+ /// Discovery-harness test (see `NATIVE_S3A_CONFIG_PROPERTIES`'s doc):
mechanically re-derives
+ /// the set of `fs.s3a.*` property suffixes this file actually resolves by
scanning this
+ /// file's OWN source text (via `include_str!`) for every
`get_config(configs, bucket, ...)`/
+ /// `get_config_trimmed(configs, bucket, ...)` call site, resolving an
identifier argument
+ /// (e.g. `PROVIDER_CLASS_PROPERTY`) through its own `const NAME: &str =
"..."` definition, and
+ /// asserts the result is EXACTLY `NATIVE_S3A_CONFIG_PROPERTIES`. This
fails loudly the moment
+ /// a call site is added, removed, or its literal changes without updating
that constant --
+ /// which is exactly the class of bug (a config key silently added to one
side of the
+ /// Scala/Rust boundary but not the other) that let a Hadoop-side
resolution rule diverge
+ /// unnoticed in the round-15 SSE-C finding.
+ ///
+ /// The `configs, property` call inside `get_config_trimmed`'s own body (a
passthrough of its
+ /// own `property` parameter, not a call site naming a fixed config key)
is deliberately
+ /// excluded by name.
+ #[test]
+ fn native_s3a_config_properties_matches_call_sites() {
Review Comment:
> Would it make sense to run that one discovery-harness test in the PR tier
whenever `native/core/src/parquet/objectstore/**` changes, so the first signal
is not the queue? Or could the contrib comparator be made advisory for keys it
does not need to compare?
I went a third way. The comparator has to stay strict, since a key native
reads but the claim gate doesn't compare is how a claimed scan ends up with a
different endpoint or credentials than Hadoop. And running the Scala test on
PRs means a `-Pdelta` build for every objectstore change.
32eb96066 puts the check in the Rust tests instead.
`delta_contrib_compares_every_native_s3a_property` reads `S3ConfigKeyConsumers`
from `DeltaScanSupport.scala` as text and fails when a key in
`NATIVE_S3A_CONFIG_PROPERTIES` is missing. The PR run that asks for the
constant update now names the Delta list too, with no new job. For the other
direction, a suite test checks that the same text parse equals the compiled
`AllS3ConfigKeys`, so a Delta-only reformat of that block fails in its own
queue run. What text can't check is whether a new key got the right consumer
tier, so the failure message says how to pick it.
Separately, `claim runs before core's metadata-column guard` fails on this
head with `column types must match schema types ... "dataChange"`. That's
#6334: it passes twice with #6458 applied and fails twice without it. So
`delta_3_5` in the queue needs #6458 to land first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
