importccl: support option to compress output files using gzip #45978

C0rWin · 2020-03-11T10:18:02Z

This commit extends EXPORT functionality by enabling compression of the
exported stream as suggested in #45579. Currently only gzip is supported
and the export clause to use compression looks as following:

export into csv 's3://export.csv' with compression = gzip from select * from foo;

Signed-off-by: Artem Barger [email protected]

Release note (sql change): support option to compress output files using
gzip

cockroach-teamcity · 2020-03-11T10:19:09Z

This change is

C0rWin · 2020-03-11T10:21:35Z

Currently still work in progress

dt

This looks great! Thanks for picking it up!

A few comments inline.

pkg/ccl/importccl/exportcsv.go

pkg/sql/execinfrapb/processors_bulk_io.proto

pkg/sql/export.go

pkg/sql/distsql_physical_planner.go

dt · 2020-03-11T13:53:30Z

As far as testing, i think you can just duplicate one of the existing tests, for example:
https://github.com/cockroachdb/cockroach/blob/master/pkg/ccl/importccl/exportcsv_test.go#L181
but just confirm that a) the EXPORT runs with your option and b) you then open a file named "n1.0.csv*.gz*" and that that file decompresses to the expected content via a gzip stream.

C0rWin · 2020-03-12T21:47:42Z

As far as testing, i think you can just duplicate one of the existing tests, for example:
https://github.com/cockroachdb/cockroach/blob/master/pkg/ccl/importccl/exportcsv_test.go#L181
but just confirm that a) the EXPORT runs with your option and b) you then open a file named "n1.0.csv*.gz*" and that that file decompresses to the expected content via a gzip stream.

thanks will add UT based on that example.

dt

This looks great! Just a few final nits inline.

I think you can ignore the red CI for the moment: the testrace failure is known flake (#45919) and the lint failure is just enforcing the 2 week code freeze.

pkg/ccl/importccl/exportcsv.go

dt · 2020-03-13T02:46:26Z

pkg/ccl/importccl/exportcsv.go

@@ -160,12 +245,16 @@ func (sp *csvWriter) Run(ctx context.Context) {
 			}
 			defer es.Close()

-			size := buf.Len()
+			size := writer.Len()


don't we need to move this after .Close() below to include the flushed buf / trailer?

good point, yes, it has to be after close to count on footers as well.

pkg/ccl/importccl/exportcsv.go

pkg/sql/execinfrapb/processors_bulk_io.proto

pkg/sql/export.go

pkg/ccl/importccl/exportcsv.go

C0rWin · 2020-03-13T17:59:10Z

I think you can ignore the red CI for the moment: the testrace failure is known flake (#45919) and the lint failure is just enforcing the 2 week code freeze.

understood and make sense 👍

dt · 2020-03-13T18:22:45Z

Thank for taking this on!

Code looks good to go once master re-opens, though it looks like you might need to make again and amend to check in the generated proto diffs.

While you're at it, if you were so inclined, could you amend the commit message so the initial line has a colon after the package, and so it mentions EXPORT e.g. importccl: support option to compress EXPORT files using gzip.

Other than that though, I think this can merge once we re-open master in the next week or two to new features. Thanks again!

C0rWin · 2020-03-21T21:41:56Z

it seems that I've mistakenly forced push from master rather than development branch, which causes to auto-close pull-request. at any rate that wasn't intentional, pushed alternative one here: #46405

managed to update and reopen

dt · 2020-03-31T02:46:28Z

@C0rWin with the 20.1 branch cut today, we're about ready to re-open master to merging feature PRs. If you are some inclined, want to give this a quick rebase (and run make to ensure the protos are re-generated since it looks like there are some diffs there right now) and we can merge it?

C0rWin · 2020-04-01T08:14:56Z

@C0rWin with the 20.1 branch cut today, we're about ready to re-open master to merging feature PRs. If you are some inclined, want to give this a quick rebase (and run make to ensure the protos are re-generated since it looks like there are some diffs there right now) and we can merge it?

let me rebase and try.

C0rWin · 2020-04-01T11:28:58Z

@dt running make leaves me with generated

        modified:   pkg/sql/execinfrapb/processors_bulk_io.pb.go
        modified:   pkg/storage/enginepb/mvcc3.pb.go
        modified:   pkg/util/hlc/timestamp.pb.go

also seems after rebasing UT fails in TeamCity due to some issue with protos.

dt · 2020-04-01T16:02:01Z

yeah, I think you want to check in those diffs as well -- a change to one proto can change all the generated ones and you just need to check in the generated diff as well. The CI is failing in the check that re-running generation produces no diffs.

C0rWin · 2020-04-02T00:56:23Z

Unit test fails due to:

panic: event RetriableErr{CanAutoRetry:false, IsCommit:false} inappropriate in current state NoTxn{} [recovered]
	panic: panic while executing 1 statements: UPDATE _._ SET _ = $1 WHERE _ = $2; caused by event RetriableErr{CanAutoRetry:false, IsCommit:false} inappropriate in current state NoTxn{}

test in failure TestBackupRestoreWithConcurrentWrites, wonder how and whenever it actually related to current PR? fyi @dt

does it make sense to just re-run UT?

This commit extends EXPORT functionality by enabling compression of the exported stream as suggested in cockroachdb#45579. Currently only gzip is supported and the export clause to use compression looks as following: ``` export into csv 's3://export.csv' with compression = gzip from select * from foo; ``` Signed-off-by: Artem Barger <[email protected]> Release note (sql change): support option to compress output files using gzip Release justification: none

dt · 2020-04-02T14:19:06Z

Whoops, sorry, the TestBackupRestoreWithConcurrentWrites failure is an unrelated flake recently introduced and fixed yesterday in #46829 -- might need one more rebase.

dt · 2020-04-02T14:20:02Z

Oh, nevermind, looks like it passed!

Thanks again for sticking with this though a rough couple weeks in our dev cycle.

bors r=dt

craig · 2020-04-02T15:06:19Z

Build succeeded

GitHub CI (Cockroach)

C0rWin mentioned this pull request Mar 11, 2020

EXPORT: support option to compress output files using gzip #45579

Closed

C0rWin force-pushed the export branch 2 times, most recently from 22a6005 to 713ca1d Compare March 11, 2020 10:35

dt self-requested a review March 11, 2020 13:34

dt reviewed Mar 11, 2020

View reviewed changes

C0rWin force-pushed the export branch 2 times, most recently from 1d20e51 to 01cafe2 Compare March 12, 2020 21:45

C0rWin force-pushed the export branch from 01cafe2 to ec2425b Compare March 12, 2020 22:20

dt approved these changes Mar 13, 2020

View reviewed changes

C0rWin force-pushed the export branch 4 times, most recently from 1783eff to 2c303b8 Compare March 13, 2020 17:57

dt self-assigned this Mar 13, 2020

dt changed the title ~~wip/importccl support option to compress output files using gzip~~ importccl: support option to compress output files using gzip Mar 13, 2020

C0rWin force-pushed the export branch 2 times, most recently from 2cabe80 to 0f03079 Compare March 13, 2020 19:14

C0rWin closed this Mar 20, 2020

C0rWin force-pushed the export branch from 0f03079 to 8eff499 Compare March 20, 2020 17:42

C0rWin deleted the export branch March 21, 2020 21:39

C0rWin reopened this Mar 21, 2020

C0rWin force-pushed the export branch from 4b78863 to 3ff86ab Compare April 1, 2020 10:41

C0rWin force-pushed the export branch 3 times, most recently from 39886d5 to 9a981cc Compare April 2, 2020 00:32

C0rWin force-pushed the export branch from 9a981cc to 6ae660b Compare April 2, 2020 00:58

craig bot merged commit 6b4b508 into cockroachdb:master Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

importccl: support option to compress output files using gzip #45978

importccl: support option to compress output files using gzip #45978

C0rWin commented Mar 11, 2020 •

edited

Loading

cockroach-teamcity commented Mar 11, 2020

C0rWin commented Mar 11, 2020

dt left a comment

dt commented Mar 11, 2020

C0rWin commented Mar 12, 2020

dt left a comment

dt Mar 13, 2020

C0rWin Mar 13, 2020

C0rWin commented Mar 13, 2020

dt commented Mar 13, 2020

C0rWin commented Mar 21, 2020 •

edited

Loading

dt commented Mar 31, 2020

C0rWin commented Apr 1, 2020

C0rWin commented Apr 1, 2020

dt commented Apr 1, 2020

C0rWin commented Apr 2, 2020 •

edited

Loading

dt commented Apr 2, 2020

dt commented Apr 2, 2020

craig bot commented Apr 2, 2020

importccl: support option to compress output files using gzip #45978

importccl: support option to compress output files using gzip #45978

Conversation

C0rWin commented Mar 11, 2020 • edited Loading

cockroach-teamcity commented Mar 11, 2020

C0rWin commented Mar 11, 2020

dt left a comment

Choose a reason for hiding this comment

dt commented Mar 11, 2020

C0rWin commented Mar 12, 2020

dt left a comment

Choose a reason for hiding this comment

dt Mar 13, 2020

Choose a reason for hiding this comment

C0rWin Mar 13, 2020

Choose a reason for hiding this comment

C0rWin commented Mar 13, 2020

dt commented Mar 13, 2020

C0rWin commented Mar 21, 2020 • edited Loading

dt commented Mar 31, 2020

C0rWin commented Apr 1, 2020

C0rWin commented Apr 1, 2020

dt commented Apr 1, 2020

C0rWin commented Apr 2, 2020 • edited Loading

dt commented Apr 2, 2020

dt commented Apr 2, 2020

craig bot commented Apr 2, 2020

Build succeeded

C0rWin commented Mar 11, 2020 •

edited

Loading

C0rWin commented Mar 21, 2020 •

edited

Loading

C0rWin commented Apr 2, 2020 •

edited

Loading