andygrove commented on code in PR #5365:
URL: https://github.com/apache/datafusion-comet/pull/5365#discussion_r4156598511
##########
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:
This test scans `s3.rs` as text and fails when a `get_config` call site is
added without updating `NATIVE_S3A_CONFIG_PROPERTIES`. `DeltaScanContribSuite`
then parses the same constant and requires `DeltaScanSupport.AllS3ConfigKeys`
to cover it. So someone who adds an S3 option in core fixes the Rust test, gets
a green PR run, and then fails in the merge queue where `delta_3_5` first runs,
until they also edit the Delta contrib. That evicts their PR and blocks the
queue for others. 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?
--
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]
