importer: use new parquet writer for EXPORT INTO PARQUET #104234

jayshrivastava · 2023-06-01T20:58:15Z

roachtest: add test for EXPORT INTO PARQUET

This change adds a rochtest which sets up a 3-node CRDB cluster
on 8vCPU machines initialized with the TPC-C database containing
250 warehouses. Then, it executes 30 EXPORT INTO PARQUET statements
concurrently, repeatedly for 10 minutes. The main purpose of this
test is to use it for benchmarking. It sets up grafana as well
so metrics can be observed during the test.

This change also adds a daily roachtest which runs exports
on a 100 warehouse TPCC database, without setting up grafana.

Informs: #103317
Release note: None

importer: use new parquet writer for EXPORT INTO PARQUET

Previously, EXPORT INTO PARQUET used a library which had
very poor memory usage. This change updates this command to
use the new parquet writer in util/parquet, which is newer
and does not use excessive memory.

To ensure backwards compatability, as much testing code as
possible is left untouched to ensure the old EXPORT INTO PARQUET
behavior is the same when using the new library. Some of the the
old production code and dependencies are also left untouched
because they are used in the test code.

This commit leaves some TODOs to clean up the tests and production
code once we have confidence in the new library.

Informs: #104329
Informs: #104278
Informs: #104279
Closes: #103317

Release note (general change): EXPORT INTO PARQUET will now
use a new internal implementation for writing parquet files
using parquet spec version 2.6. There should be no significant
impact to the structure of files being written. There is one
minor change:

all columns written to parquet files will be nullable
(ie. the parquet repetition type is OPTIONAL)

cockroach-teamcity · 2023-06-01T20:58:28Z

This change is

jayshrivastava

I posted the memstats here: https://drive.google.com/drive/folders/1xIevGiHsC95jnsQQF3Y1N5kSzltKMo1w?usp=share_link

In summary, I ran the roachtest and found that EXPORT with the new library uses

42% less memory (1703707920B / 2948454120B). This is a difference 1.24 GB when running concurrent exports on a 14GB database.
72% fewer allocs (2598922890 / 9259896304)

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/importer/exportparquet_test.go line 140 at r2 (raw file):

		require.Equal(t, expected.(*tree.DJSON).JSON.String(),
			actual.(*tree.DJSON).JSON.String())
	case types.EnumFamily:

Before I added these changes, we would not write the additional metadata (ex. collation) to parquet files. The metadata was a part of the encoders and decoders here:

cockroach/pkg/sql/importer/exportparquet.go

Line 163 in 826809e

    
           return tree.NewDCollatedString(string(x.([]byte)), typ.Locale(), &tree.CollationEnvironment{})

IMO if we aren't writing such things to files, then there is no point in testing them.

miretskiy

Reviewed 3 of 3 files at r1, 5 of 7 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @herkolategan, @jayshrivastava, and @smg260)

go.mod line 216 at r3 (raw file):

	github.com/stretchr/testify v1.8.3
	github.com/twpayne/go-geom v1.4.2
	github.com/uber-go/atomic v1.4.0

maybe we shouldn't add this dependency? What does uber-go/atomic has that atomic does not?

pkg/cmd/roachtest/tests/export_parquet.go line 35 at r3 (raw file):

		Name:            "export/parquet",
		Owner:           registry.OwnerCDC,
		Tags:            registry.Tags("manual"),

do we want to have smaller non-manual test? Or does it exist?

pkg/sql/importer/exportparquet.go line 37 at r3 (raw file):

	"github.com/cockroachdb/cockroach/pkg/util/bitarray"
	"github.com/cockroachdb/cockroach/pkg/util/duration"
	crlparquet "github.com/cockroachdb/cockroach/pkg/util/parquet"

add todo or just do it here -- we need memory monitor for this stuff.

pkg/sql/importer/exportparquet.go line 643 at r3 (raw file):

			compression = crlparquet.CompressionGZIP
		case roachpb.IOFileFormat_Auto, roachpb.IOFileFormat_None:
			compression = crlparquet.CompressionNone

crlparquet supports other compression formats, right?
Perhaps add an issue/todo to make export support those too.
Also, todo to refactor string -> codec parsing.

pkg/sql/importer/exportparquet_test.go line 145 at r3 (raw file):

			actual.(*tree.DEnum).LogicalRep)
	case types.CollatedStringFamily:
		// Only the value of the collated string matters, not that additional properties

and it does appear that parquet does not have notion of collation... am I correct?

jayshrivastava

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @herkolategan, @miretskiy, and @smg260)

go.mod line 216 at r3 (raw file):