Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort output of S3_File.list and Enso_File.list #11929

Merged
merged 25 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
cleanup
  • Loading branch information
GregoryTravis committed Dec 19, 2024
commit 4ebf3f3e072c73c335c5c43e525e6dacad51f3e9
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ from Standard.Base import all
import Standard.Base.Errors.Illegal_Argument.Illegal_Argument
import Standard.Base.Internal.Ordering_Helpers.Default_Comparator
import Standard.Base.Internal.Path_Helpers
from Standard.Base.Data.Ordering import comparable_via

import project.Errors.S3_Error
import project.S3.S3
Expand Down Expand Up @@ -160,10 +159,8 @@ type Decomposed_S3_Path
Decomposed_S3_Path.Value new_parts self.go_to_root

S3_Path_Comparator =
compare x y =
Ordering.compare [x.bucket, x.key] [y.bucket, y.key]
compare x y = Ordering.compare [x.bucket, x.key] [y.bucket, y.key]

hash x = Default_Comparator.hash_builtin x

#Comparable.from that:S3_Path = Comparable.new that S3_Path_Comparator
Comparable.from that:S3_Path = comparable_via (p-> [p.bucket, p.key])
Comparable.from that:S3_Path = Comparable.new that S3_Path_Comparator
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Comparable.from that:S3_Path = Comparable.new that S3_Path_Comparator
## PRIVATE
Comparable.from that:S3_Path = Comparable.new that S3_Path_Comparator

Copy link
Member

Choose a reason for hiding this comment

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

;-)

It would be easier to change the IDE to not display from methods in the component browser than adding this ## PRIVATE at every conversion method occurrence.

13 changes: 2 additions & 11 deletions distribution/lib/Standard/AWS/0.0.0-dev/src/S3/S3_File.enso
Original file line number Diff line number Diff line change
Expand Up @@ -179,16 +179,15 @@ type S3_File
check_recursion action = if recursive then Unimplemented.throw "S3 listing with recursion is not currently implemented." else action
check_directory action = if self.is_directory.not then Error.throw (Illegal_Argument.Error "Cannot `list` a non-directory." self.uri) else action

results = check_directory <| check_recursion <| check_name_filter <|
check_directory <| check_recursion <| check_name_filter <|
if self.s3_path.bucket == "" then translate_file_errors self <| S3.list_buckets self.credentials . map bucket-> S3_File.Value (S3_Path.Value bucket "") self.credentials else
pair = translate_file_errors self <| S3.read_bucket self.s3_path.bucket self.s3_path.key self.credentials delimiter=S3_Path.delimiter
bucket = self.s3_path.bucket
sub_folders = pair.first . map key->
S3_File.Value (S3_Path.Value bucket key) self.credentials
files = pair.second . map key->
S3_File.Value (S3_Path.Value bucket key) self.credentials
sub_folders + files
results.sort on=.path
sub_folders + files . sort on=.s3_path
Copy link
Member

Choose a reason for hiding this comment

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

Won't this just sort files but not sub_folders?

Copy link
Member

Choose a reason for hiding this comment

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

Also we could just sort on .path and then we wouldn't need to implement the custom comparator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better to not use a comparator? I was thinking of adding a comparator for S3_File as well -- if a user might have a vector of them, then they might want to sort it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like it sorted the files and sub-folders properly:

s3://enso-data-samples/Store Data.xlsx
s3://enso-data-samples/data_2500_rows.csv
s3://enso-data-samples/examples/
s3://enso-data-samples/locations.json
s3://enso-data-samples/products.csv
s3://enso-data-samples/sample.xml
s3://enso-data-samples/spreadsheet.xls
s3://enso-data-samples/spreadsheet.xlsx
s3://enso-data-samples/tableau/
s3://enso-data-samples/transactions.csv

Copy link
Member

Choose a reason for hiding this comment

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

I'd still add parentheses, the precedence of + vs . is not something that everyone knows immediately, so I think making it clear with parentheses will make it better.

Copy link
Member

Choose a reason for hiding this comment

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

Is it better to not use a comparator? I was thinking of adding a comparator for S3_File as well -- if a user might have a vector of them, then they might want to sort it.

Hmm, I guess we could do that yeah. But still I'd rely on comparing the path as Text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


## ALIAS load bytes, open bytes
ICON data_input
Expand Down Expand Up @@ -617,11 +616,3 @@ translate_file_errors related_file result =
s3_path = S3_Path.Value error.bucket error.key
s3_file = S3_File.Value s3_path related_file.credentials
Error.throw (File_Error.Not_Found s3_file)

## PRIVATE
Sort S3 files by bucket and path.
sort_s3_files s3_files =
s3_file_sort_key s3_file =
s3_path = s3_file.path
[s3_path.bucket, s3.path.key]
s3_files.sort on=s3_file_sort_key
Original file line number Diff line number Diff line change
Expand Up @@ -193,14 +193,5 @@ type Ordering
from_sign sign = if sign == 0 then Ordering.Equal else
if sign > 0 then Ordering.Greater else Ordering.Less

type Via_Comparator
Value f

compare self x y = Ordering.compare (self.f x) (self.f y)

hash x = Default_Comparator.hash_builtin x

comparable_via that f = Comparable.new that (Via_Comparator.Value f)

## PRIVATE
Comparable.from (that:Ordering) = Comparable.new that Ordering_Comparator
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import project.Internal.Ordering_Helpers.Default_Comparator
import project.Internal.Path_Helpers
import project.Nothing.Nothing
from project.Data.Boolean import Boolean, False, True
from project.Data.Ordering import comparable_via
from project.Data.Text.Extensions import all

## PRIVATE
Expand Down Expand Up @@ -77,11 +76,9 @@ normalize segments =
that points to the user home and it should be correctly normalized, but requires numerous passes to do so.
@Tail_Call normalize new_segments

type Enso_File_Comparator
compare x y =
Ordering.compare x.path_segments y.path_segments
type Enso_Path_Comparator
compare x y = Ordering.compare x.path_segments y.path_segments

hash x = Default_Comparator.hash_builtin x

#Comparable.from that:Enso_File = Comparable.new that Enso_File_Comparator
Comparable.from that:Enso_File = comparable_via that .path_segments
Comparable.from that:Enso_Path = Comparable.new that Enso_Path_Comparator
3 changes: 2 additions & 1 deletion test/AWS_Tests/src/S3_Spec.enso
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,8 @@ add_specs suite_builder =
group_builder.specify "list should sort its output" <|
r = root.list
r.should_be_a Vector
r . should_equal r.sort
r . should_equal (r.sort on=.s3_path)
r . should_equal (r.sort on=(x-> x.s3_path.to_text))

group_builder.specify "will fail if no credentials are provided and no Default credentials are available" pending=(if AWS_Credential.is_default_credential_available then "Default AWS credentials are defined in the environment and this test has no way of overriding them, so it is impossible to test this scenario in such environment.") <|
root_without_credentials = S3_File.new "s3://"+bucket_name+"/"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ add_specs suite_builder setup:Cloud_Tests_Setup = suite_builder.group "Enso Clou
group_builder.specify "list should sort its output" <|
r = Enso_File.home.list
r.should_be_a Vector
r . should_equal r.sort
r . should_equal (r.sort on=.path)
r . should_equal (r.sort on=.enso_path)
Copy link
Member

Choose a reason for hiding this comment

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

enso_path is private, I don't think the second check can work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.


group_builder.specify "should allow to create and delete a directory" <|
my_name = "my_test_dir-" + (Random.uuid.take 5)
Expand Down