Optimize HiveSplit serialization by arhimondr · Pull Request #13453 · prestodb/presto

arhimondr · 2019-09-26T00:51:17Z

Serializing "schema" for every splits is very expensive. Instead of creating it on the coordinator it can be re-constructed on a worker.

If release note is NOT required, use:

== RELEASE NOTES ==

Hive Changes
* Improve cpu load on coordinator by reducing the cost of serializing ``HiveSplit``s

rschlussel · 2019-09-26T17:12:22Z

haven't read the PR yet, but this should get a release note since it fixes a performance issue.

presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetPageSourceFactory.java

presto-hive/src/main/java/com/facebook/presto/hive/BackgroundHiveSplitLoader.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveMetadata.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveTableLayoutHandle.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitPartitionInfo.java

rschlussel · 2019-09-27T15:45:10Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveType.java

why don't you account for the size of hiveTypeName

after in person conversation it's because the values are shared from a cache. So the plan is

add a note explaining.

get rid of dead code in HiveTypeName if HiveTypeName.getEstimatedSizeInBytes isn't used anywhere else

The class has two filelds

private final HiveTypeName hiveTypeName; private final TypeInfo typeInfo;

hiveTypeName is set from typeInfo

TypeInfo objects are always created by the TypeInfoFactory that has a static singleton cache. Thus it doesn't make much sense to account memory for these objects here, as those are shared.

hiveTypeName is set from typeInfo

Sorry, that is not true. HiveTypeName is created based on TypeInfo. We still need to account for it. Let me fix it.

Wow, does that mean Column is not counted for memory usage for years?

BTW: why the method name is not getEstimatedRetainedSizeInBytes?

Wow, does that mean Column is not counted for memory usage for years?

In this PR the Map<Integer, HiveTypeName> columnCoercion is replaced with the Map<Integer, Column>, that's why the size of the Column has to be accounted.

BTW: why the method name is not getEstimatedRetainedSizeInBytes?

I believe the question is why it is not the getEstimatedSizeInBytes. I wanted the name to be kinda descriptive of the fact that the size of the TypeInfo is not accounted, as it is not "retained" by the Column object.

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplitManager.java

rschlussel

Feel free to merge after addressing the remaining comments

arhimondr · 2019-09-27T21:56:38Z

@rschlussel comments addressed

wenleix

"Refactor HiveTableLayoutHandle" LGTM.

wenleix

"Optimize imports in ParquetPageSourceFactory": This is accidentally done in #13473 so I think you can drop it ;)

wenleix

I made an initial pass and generally looks good. I will made another pass next week.

I have two questions:

The idea is we don't need to have partition schema in each HiveSplit, instead, we can recompute it via table schema + column coercion. Is it guaranteed to be the same ?
Should we use the old name (columnCoercion), or use the new name (partitionSchemaDifference or partitionSchemaOverride). The former is more compatible with the Hive context and other existing enum/variables (e.g. CoercionPolicy). The latter seems to be more descriptive.

wenleix · 2019-09-28T04:55:21Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveType.java

Wow, does that mean Column is not counted for memory usage for years?

BTW: why the method name is not getEstimatedRetainedSizeInBytes?

wenleix · 2019-09-28T04:58:24Z

presto-hive/src/main/java/com/facebook/presto/hive/metastore/MetastoreUtil.java

FWIW, sd comes from Hive which is the abbreviation for StorageDescriptor

Yeah. That makes sense. However i found that storage would be a better name for the variable of type Storage, so it is not confused with the StorageDescriptor.

wenleix · 2019-09-28T05:02:56Z

presto-hive/src/main/java/com/facebook/presto/hive/metastore/MetastoreUtil.java

What about calling the third parameter partitionSchmeaOverride? -- basically it defines "column overrides" at partition level right?

Let me call it partitionSchemaDifference, to be consistent with the field name in the HiveSplit

wenleix · 2019-09-28T05:03:52Z

presto-hive/src/main/java/com/facebook/presto/hive/orc/DwrfBatchPageSourceFactory.java

nit: is this just an inline? -- do we need this change?

The Properties schema is no longer in the parameters. Now there's Storage instead.

wenleix · 2019-09-28T05:06:11Z

presto-hive/src/main/java/com/facebook/presto/hive/BackgroundHiveSplitLoader.java

nit:

int partitionDataColumnCount = partition.getPartition() .map(p -> p.getColumns().size()) .orElse(table.getDataColumns().size());

wenleix · 2019-09-28T05:06:47Z

presto-hive/src/main/java/com/facebook/presto/hive/metastore/MetastoreUtil.java

Not quite sure understand this method.

This is needed in the BackgroundHiveSplitLoader to avoid creating the whole schema, as it only needs custom properties from the storage descriptor with a possible override by table properties. The semantic is weird, but I'm just copying the existing behaviour.

wenleix · 2019-09-28T05:10:33Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplit.java

nit: What about name this partitionSchmeaOverride ? Ditto for other places.

Than it should be something like partitionColumnSchemaOverrides. As this doesn't override the whole partition schema, but only some columns. Thus i like the partitionSchemaDifference more. I would like to keep it that way if you don't mind.

wenleix · 2019-09-28T05:11:53Z

presto-hive/src/main/java/com/facebook/presto/hive/HivePageSourceProvider.java

I will let @rschlussel decide whether this variable should be named coercionFrom or coerceFrom 😃

I didn't mean to rename it. Renamed it back.

wenleix · 2019-09-28T05:12:36Z

presto-hive/src/main/java/com/facebook/presto/hive/HiveSplit.java

Curious: why we need to pass Storage now? Can serdeParameter be a huge map?

Also what's suppose to be stored in schema when it's a Properties

Curious: why we need to pass Storage now? Can serdeParameter be a huge map?

This object contains partition specific storage information. Unfortunately if partition has some custom storage parameter - we should pass it, as those are needed to initialize the Input reader / serde.

Also what's suppose to be stored in schema when it's a Properties

Yeah, the Properties schema used to contain them before.

Generate table layout name outside of the HiveTableLayoutHandle constructor.

arhimondr · 2019-09-30T17:05:01Z

@wenleix

The idea is we don't need to have partition schema in each HiveSplit, instead, we can recompute it via table schema + column coercion. Is it guaranteed to be the same ?

Unless there's a bug it should be the same

Should we use the old name (columnCoercion), or use the new name (partitionSchemaDifference or partitionSchemaOverride). The former is more compatible with the Hive context and other existing enum/variables (e.g. CoercionPolicy). The latter seems to be more descriptive.

Since the partition schema has to be recreated now, the Map<Integer, HiveTypeName> columnCoercion is not enough, as it contains information only about the difference it types. To accurately recreated the partition schema - the difference in column names should also be tracked. That's why the Map<Integer, HiveTypeName> got replaced with the Map<Integer, Column>. And that's why the Map<Integer, Column> also contains the information about the extra columns present in partition (the Map<Integer, HiveTypeName> didn't)

Then based on the partitionSchemaDifference the column mappings for coercion policy are created.

wenleix

LGTM.

wenleix · 2019-09-30T22:51:09Z

presto-hive/src/main/java/com/facebook/presto/hive/metastore/MetastoreUtil.java

-    private static Properties getHiveSchema(
-            Storage sd,
-            List<Column> dataColumns,
+    public static Properties getHiveSchema(


We might want to rename this method into something like getHiveTableParameters in a separate PR.

This is actually not gonna be precise. As the Properties getHiveSchema follows this weird semantic of replacing null values with empty strings. This method is trying to mimic the behaviour of the original getHiveSchema

facebook-github-bot added the CLA Signed label Sep 26, 2019

arhimondr force-pushed the optimize_hive_split branch from 4020ac5 to c1389a7 Compare September 26, 2019 15:58

arhimondr changed the title ~~[WIP] Optimize HiveSplit serialization~~ Optimize HiveSplit serialization Sep 26, 2019

arhimondr requested review from rschlussel and wenleix September 26, 2019 15:59

arhimondr assigned wenleix and rschlussel and unassigned wenleix Sep 26, 2019

arhimondr added the RELEASE-BLOCKER label Sep 26, 2019

rschlussel reviewed Sep 27, 2019

View reviewed changes

rschlussel approved these changes Sep 27, 2019

View reviewed changes

arhimondr force-pushed the optimize_hive_split branch from c1389a7 to 4559325 Compare September 27, 2019 21:53

wenleix reviewed Sep 28, 2019

View reviewed changes

wenleix assigned arhimondr and unassigned wenleix and rschlussel Sep 28, 2019

Refactor HiveTableLayoutHandle

1402ae9

Generate table layout name outside of the HiveTableLayoutHandle constructor.

Remove schema from the HiveSplit

5c822fa

arhimondr force-pushed the optimize_hive_split branch from 4559325 to 5c822fa Compare September 30, 2019 17:20

wenleix approved these changes Sep 30, 2019

View reviewed changes

arhimondr merged commit 1d61bc7 into prestodb:master Oct 1, 2019

arhimondr deleted the optimize_hive_split branch October 1, 2019 00:00

yingsu00 mentioned this pull request Oct 2, 2019

Release notes for 0.227 #13490

Closed

3 tasks

arhimondr mentioned this pull request Dec 13, 2021

Provide memory tracking capabilities for connector splits trinodb/trino#10273

Merged

arhimondr mentioned this pull request Dec 22, 2022

Optimize HiveSplit trinodb/trino#15511

Closed

Conversation

arhimondr commented Sep 26, 2019 • edited by rschlussel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rschlussel commented Sep 26, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rschlussel left a comment

Choose a reason for hiding this comment

Uh oh!

arhimondr commented Sep 27, 2019

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arhimondr commented Sep 30, 2019

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

arhimondr commented Sep 26, 2019 •

edited by rschlussel

Loading