You are viewing a plain text version of this content. The canonical link for it is here.

Posted to github@arrow.apache.org by "esheppa (via GitHub)" <gi...@apache.org> on 2024/02/12 23:49:07 UTC

[PR] Partitioning fixes [arrow-datafusion]

esheppa opened a new pull request, #9207:
URL: https://github.com/apache/arrow-datafusion/pull/9207

   ## Which issue does this PR close?
   
   Closes #9206.
   
   ## Rationale for this change
   
   * Aligns the code behaviour with the documentation
   
   
   ## What changes are included in this PR?
   
   * Altering the `ListingTableUrl::contains` method to skip hive partitions where they appear as a segment in the url
   * Makes the `list_all_files` method public to allow users to more easily see what files are being included with the `ListingTableUrl`
   
   ## Are these changes tested?
   
   Yes a new test is added to ensure that the changes continue to behave as expected
   
   ## Are there any user-facing changes?
   
   * An additional public function on `ListingTableUrl`
   * Extra files may be listed for a given `ListingTableUrl` which previously were not included. This could cause query results to change
   


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Partitioning fixes [arrow-datafusion]

Posted by "esheppa (via GitHub)" <gi...@apache.org>.

esheppa commented on code in PR #9207:
URL: https://github.com/apache/arrow-datafusion/pull/9207#discussion_r1489309436


##########
datafusion/core/src/datasource/listing/url.rs:
##########
@@ -189,34 +190,39 @@ impl ListingTableUrl {
 
     /// Returns `true` if `path` matches this [`ListingTableUrl`]
     pub fn contains(&self, path: &Path, ignore_subdirectory: bool) -> bool {
-        match self.strip_prefix(path) {
-            Some(mut segments) => match &self.glob {
-                Some(glob) => {
-                    if ignore_subdirectory {
-                        segments
-                            .next()
-                            .map_or(false, |file_name| glob.matches(file_name))
-                    } else {
-                        let stripped = segments.join("/");
-                        glob.matches(&stripped)
-                    }
+        let Some(all_segments) = self.strip_prefix(path) else {
+            return false;
+        };
+
+        // remove any segments that contain `=` as they are allowed even
+        // when ignore subdirectories is `true`.
+        let mut segments = all_segments.filter(|s| !s.contains('='));
+
+        match &self.glob {
+            Some(glob) => {
+                if ignore_subdirectory {
+                    segments
+                        .next()
+                        .map_or(false, |file_name| glob.matches(file_name))
+                } else {
+                    let stripped = segments.join(DELIMITER);
+                    glob.matches(&stripped)
                 }
-                None => {
-                    if ignore_subdirectory {
-                        let has_subdirectory = segments.collect::<Vec<_>>().len() > 1;
-                        !has_subdirectory
-                    } else {
-                        true
-                    }
+            }
+            None => {
+                if ignore_subdirectory {
+                    let has_subdirectory = segments.count() > 1;
+                    !has_subdirectory
+                } else {
+                    true
                 }
-            },
-            None => false,
+            }
         }
     }
 
     /// Returns `true` if `path` refers to a collection of objects
     pub fn is_collection(&self) -> bool {
-        self.url.as_str().ends_with('/')
+        self.url.as_str().ends_with(DELIMITER)
     }

Review Comment:
   updated this to use the `DELIMITER` as elsewhere



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Partitioning fixes [arrow-datafusion]

Posted by "esheppa (via GitHub)" <gi...@apache.org>.

esheppa commented on code in PR #9207:
URL: https://github.com/apache/arrow-datafusion/pull/9207#discussion_r1489247669


##########
datafusion/core/src/datasource/listing/url.rs:
##########
@@ -234,7 +239,7 @@ impl ListingTableUrl {
     }
 
     /// List all files identified by this [`ListingTableUrl`] for the provided `file_extension`
-    pub(crate) async fn list_all_files<'a>(
+    pub async fn list_all_files<'a>(

Review Comment:
   I'm happy to separate this out if preferred. I made this change because it allows easier debugging for future users who are unsure which files the `ListingTableUrl` will actually list. This means one can see whether the issue is with the url itself, or another issue



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org