557 import tool usability #800

johnnyaug · 2020-10-11T15:52:08Z

Implements #557

Align inventory column requirements between Parquet in ORC: make all columns except bucket and key optional in Parquet.
Add tests to parquet reader

codecov-io · 2020-10-11T16:03:47Z

Codecov Report

Merging #800 into master will increase coverage by 0.17%.
The diff coverage is 80.48%.

@@            Coverage Diff             @@
##           master     #800      +/-   ##
==========================================
+ Coverage   41.78%   41.95%   +0.17%     
==========================================
  Files         133      133              
  Lines       10332    10442     +110     
==========================================
+ Hits         4317     4381      +64     
- Misses       5433     5480      +47     
+ Partials      582      581       -1

Impacted Files	Coverage Δ
cloud/aws/s3inventory/parquet_reader.go	`76.19% <75.40%> (+76.19%)`	⬆️
cloud/aws/s3inventory/orc_reader.go	`83.33% <93.33%> (+5.91%)`	⬆️
block/s3/inventory_iterator.go	`79.71% <100.00%> (-0.29%)`	⬇️
cloud/aws/s3inventory/reader.go	`57.89% <100.00%> (+19.00%)`	⬆️
block/local/adapter.go	`5.71% <0.00%> (-0.74%)`	⬇️
block/adapter.go	`0.00% <0.00%> (ø)`
block/gs/adapter.go	`0.00% <0.00%> (ø)`
block/s3/adapter.go	`0.00% <0.00%> (ø)`
api/api_controller.go	`36.13% <0.00%> (+0.07%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c70ae53...a03a4fd. Read the comment docs.

arielshaqed

Thanks!

Very nice, much cleaner. Please go over comments but this is good and approved (almost) any way you handle comments

arielshaqed · 2020-10-12T08:53:29Z

block/s3/inventory_iterator.go

@@ -94,8 +94,7 @@ func (it *InventoryIterator) fillBuffer() bool {
 			it.logger.Errorf("failed to close manifest file reader. file=%s, err=%w", it.Manifest.Files[it.inventoryFileIndex].Key, err)
 		}
 	}()
-	it.buffer = make([]s3inventory.InventoryObject, rdr.GetNumRows())
-	err = rdr.Read(&it.buffer)
+	it.buffer, err = rdr.Read(int(rdr.GetNumRows()))


Maybe Read should take an int64?

this will cause worse casting mess

block/s3/inventory_test.go

cloud/aws/s3inventory/orc_reader.go

cloud/aws/s3inventory/parquet_reader.go

arielshaqed · 2020-10-12T09:03:47Z

cloud/aws/s3inventory/parquet_reader.go

+		var checksum string
+		checksum, err = cast.ToStringE(v)
+		o.Checksum = swag.String(checksum)
+	}


Please add a default. This is Go, there are no enums and no compile-time switch cover test for fake enums.

arielshaqed · 2020-10-12T09:04:47Z

cloud/aws/s3inventory/parquet_reader.go

 			if err != nil {
 				errChan <- fmt.Errorf("failed to read parquet column %s: %w", fieldName, err)
 				return
 			}
 			for i, v := range columnRes {
+				if dls[i] == 0 && fieldName != "key" && fieldName != "bucket" {


Not sure I understand. What is dls?

arielshaqed · 2020-10-12T11:26:59Z

cloud/aws/s3inventory/parquet_reader.go

+	case lastModifiedDateFieldName:
+		var lastModifiedMillis int64
+		lastModifiedMillis, err = cast.ToInt64E(v)
+		o.LastModifiedMillis = swag.Int64(lastModifiedMillis)


As before, would still be happier to have a proper time.Time here.

arielshaqed · 2020-10-12T11:29:35Z

cloud/aws/s3inventory/reader_test.go

 			ObjectNum:           12500,
 			ExpectedReadObjects: 12500,
 			ExpectedMinValue:    "f00000",
 			ExpectedMaxValue:    "f12499",
+			Format:              "ORC",
+		},
+		"orc with 100 objects": {


Could we just run each of the test cases twice, once with each Format?

…nge time field to time.Time

arielshaqed

Cool; thanks! StilLGTM...

arielshaqed · 2020-10-12T14:11:58Z

cloud/aws/s3inventory/parquet_reader.go

-		var checksum string
-		checksum, err = cast.ToStringE(v)
-		o.Checksum = swag.String(checksum)
+		o.LastModified = swag.Time(time.Unix(lastModifiedMillis/int64(time.Second/time.Millisecond), 0))


Do we care about losing the subsecond resolution?

Suggested change

o.LastModified = swag.Time(time.Unix(lastModifiedMillis/int64(time.Second/time.Millisecond), 0))

o.LastModified = swag.Time(time.Unix(lastModifiedMillis/int64(1000), (lastModifiedMillis % 1000) * int64(1_000_000))

(also you can see that I'm not a fan of pretending that the number of milliseconds per second may change, and I'm trying to sneak that change by you...).

Our linter would never allow it !

johnnyaug added 5 commits October 7, 2020 15:35

parquet reader: interface change, fix crash on missing fields

17d04b5

skip optional, empty fields

495d677

import tool usability+tests

4192893

fix test

715454e

order

737c4c6

order

a03a4fd

johnnyaug requested a review from arielshaqed October 12, 2020 08:27

Merge branch 'master' into 557_import_tool_usability

b60b0ae

arielshaqed approved these changes Oct 12, 2020

View reviewed changes

johnnyaug added 3 commits October 12, 2020 16:57

CR fixes + simplifications: remove pointers from InventoryObject, cha…

a1e98cb

…nge time field to time.Time

CR fix: run test once for each format

54ddca0

CR fix: add default to switch

a45bcbe

johnnyaug requested a review from arielshaqed October 12, 2020 14:09

arielshaqed approved these changes Oct 12, 2020

View reviewed changes

add sub-second part of modification time + tests

a643bd6

johnnyaug merged commit b78e5c6 into master Oct 12, 2020

johnnyaug deleted the 557_import_tool_usability branch October 12, 2020 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

557 import tool usability #800

557 import tool usability #800

johnnyaug commented Oct 11, 2020 •

edited

Loading

codecov-io commented Oct 11, 2020 •

edited

Loading

arielshaqed left a comment

arielshaqed Oct 12, 2020

johnnyaug Oct 12, 2020

arielshaqed Oct 12, 2020

arielshaqed Oct 12, 2020

arielshaqed Oct 12, 2020

arielshaqed Oct 12, 2020

arielshaqed left a comment

arielshaqed Oct 12, 2020

johnnyaug Oct 12, 2020

	o.LastModified = swag.Time(time.Unix(lastModifiedMillis/int64(time.Second/time.Millisecond), 0))
	o.LastModified = swag.Time(time.Unix(lastModifiedMillis/int64(1000), (lastModifiedMillis % 1000) * int64(1_000_000))

557 import tool usability #800

557 import tool usability #800

Conversation

johnnyaug commented Oct 11, 2020 • edited Loading

codecov-io commented Oct 11, 2020 • edited Loading

Codecov Report

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyaug commented Oct 11, 2020 •

edited

Loading

codecov-io commented Oct 11, 2020 •

edited

Loading