Allow to change compression codec with a session property #12647

arhimondr · 2019-04-11T21:32:30Z

Required by #12387

rschlussel · 2019-04-12T19:20:15Z

presto-hive/src/main/java/com/facebook/presto/hive/util/ConfigurationUtils.java

+        // This code assumes that if configuration is an instance of FileSystemFactory, a new instance is always supplied
+        if (config instanceof FileSystemFactory) {
+            checkArgument(config instanceof JobConf, "config is not an instance of JobConf: %s", config.getClass());
+            checkArgument(config.get(IS_NOT_A_COPY) == null, "config is not a copy");


i don't understand this. What's the is_not_a_copy flag, and what does the error message here mean?

also, does this commit need to get squashed into add compression_codec session property for correctness purposes?

What's the is_not_a_copy flag, and what does the error message here mean?

Basically that's a sanity check to verify that the configuration passed to this method is always a clean copy.

also, does this commit need to get squashed into add compression_codec session property for correctness purposes?

Sure, i don't mind squashing it once the review is done. Just wanted to extract it to a separate commit so i can provide a more maningful explanation in the commit message.

I'm not sure I understand. Here's what I think you're saying:
"is_not_a_copy" is your own made up flag to indicate if we are dealing with a new instance of a jobconf (a copy) or whether we have a shared instance.

if "is_not_a_copy" is true, that means we have a shared instance

if it's false then it's a new unique instance

and if it's null/unset that also means it's unique because we assume we always get a new instance from FileSystemFactory? (if this is true, the error message for the checkArgument needs to be fixed).

I still don't understand why we care. If mutating a shared JobConf is a problem, why don't we create our own copy in the method? that seems much more standard. Also, why do we set it to true next? all that we do with it is set some properties in a helper method.

Discussed offline. Decided to remove the assertion check, as it looks incredibly confusing.

wenleix

I haven't looked in details, but I have a high-level question:

It looks making compression codec configurable by session property has some unexpected complications (probably due to JobConf copy issue?). So, does it make sense to only have config properties for temporary tables? -- After all, we probably don't have to change this in a per query basis :)

We can also make Temporary Table write only use optimized ORC writer -- they are quite Presto internal so we should just fix to something works best. And hard code it to use the compression codec. (See, for example, the TempFileWriter: https://github.com/prestodb/presto/blob/9b8b08cc762a1c33ec4fec9b65018a4d18c7e9d8/presto-hive/src/main/java/com/facebook/presto/hive/util/TempFileWriter.java )

Feel free to correct me if I overlook anything (or talk in person :) )

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

arhimondr · 2019-04-15T14:03:06Z

So, does it make sense to only have config properties for temporary tables?

What I'm trying to do here - is to be able to set a different compression algorithm for a temporary table, without changing the compression algorithm for a regular table. I think that's what introduces the complication. Adding a session property on top of it doesn't change much.

The only simpler approach i can think of is to pass a CompressionCodec to the HdfsEnvironment#getConfiguration. But that seems to be weird, contrintuinive API.

wenleix · 2019-04-15T18:31:00Z

@arhimondr : What about a solution similar to this: wenleix@592f5bb ?

It's a hacky POC just to demonstrate the idea. Basically the HiveWritableTableHandle have an extra bit about whether the table is temporary, and pass this flag down to HiveWriterFactory -> HiveFileWriterFactory#createFileWriter. So if it's a temporary file it directly it will hard code to SNAPPY (and in the future it will be ZSTANDARD )

arhimondr · 2019-04-15T20:00:09Z

@wenleix I see the point. So the point is to make it work only for ORC. That might work. Though i find the solution with a session property for compression cleaner, and more useful in general.

wenleix · 2019-04-15T21:51:40Z

@arhimondr : It can also work fo RCFile -- as long as it has "native" Presto optimized writer support :) . We can still have session property, we just don't avoid go though the Hadoop JobConf stuffs :)

presto-hive/src/main/java/com/facebook/presto/hive/util/ConfigurationUtils.java

wenleix

"Add compression_codec session property in Hive connector": LGTM.

wenleix · 2019-04-19T00:03:01Z

presto-hive/src/main/java/com/facebook/presto/hive/util/ConfigurationUtils.java

+    public static JobConf configureCompression(Configuration config, HiveCompressionCodec compression)
+    {
+        JobConf result = new JobConf(false);
+        copy(config, result);


Curious, what's the difference between doing this vs.

presto/presto-hive/src/test/java/com/facebook/presto/hive/benchmark/FileFormat.java

Line 421 in 6727da1

JobConf config = new JobConf(conf);

JobConf config = new JobConf(conf);

This will re-read the configuration xml files in the classpath 😄

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

wenleix

"Refactor HiveClientConfig"
"Add temporary_table_compression_codec session property"
"Simplify HiveWriterFactory#getFileExtension"

Looks good.

wenleix · 2019-04-19T00:11:42Z

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

-        }
-        catch (RuntimeException e) {
-            throw new PrestoException(HIVE_UNSUPPORTED_FORMAT, "Failed to load compression codec: " + compressionCodecClass, e);
+        catch (ReflectiveOperationException e) {


Does that mean RuntimeException caught in the original code is too broad? (i.e. it should always catch ReflectiveOperationException)

I think so. I manually checked the constructors, and those constructors don't throw any exceptions.

wenleix

"Add workaround for FileSystemFactory"

Looks good. Maybe just change the title to

Fix configureCompression method for FileSystemFactory

"Add workaround" sounds like a hack :) .

wenleix · 2019-04-19T00:21:53Z

presto-hive/src/main/java/com/facebook/presto/hive/util/ConfigurationUtils.java

-        JobConf result = new JobConf(false);
-        copy(config, result);
+        JobConf result;
+        // Workaround for https://github.com/prestodb/presto-hadoop-apache2/commit/aa3a59101cb14659a96400a3943cd1bb740399b9


I think we usually don't have commit in code comment? What about the following (feel free to modify it)

FileSystemFactory is used to hack around the abuse of Configuration as a cache for FileSystem. See FileSystemFactory class for more details. It is caller's responsibility to create a copy if FileSystemFactory is used.

And add javadoc comment to FileSystemFactory (perhaps just copy the commit message from prestodb/presto-hadoop-apache2@aa3a591)

Yeah, i aggree that using commit ids is not what we do. I changed the commit message.

And add javadoc comment to FileSystemFactory (perhaps just copy the commit message from prestodb/presto-hadoop-apache2@aa3a591)

Do do that we need to create a PR to the presto-hadoop-apache2, release the library and update it's version. Do we really think that the comment worth it?

arhimondr · 2019-04-22T20:42:39Z

@wenleix , @rschlussel updated

Rename hiveCompressionCodec to simply compressionCodec

And hive.temporary-table-compression-codec configuration property

arhimondr requested review from wenleix, tdcmeehan and rschlussel April 11, 2019 21:32

facebook-github-bot added the CLA Signed label Apr 11, 2019

arhimondr assigned wenleix and rschlussel Apr 11, 2019

arhimondr changed the title ~~Allow to change compression codec with a session property~~ [WIP] Allow to change compression codec with a session property Apr 11, 2019

arhimondr force-pushed the change-compression-for-temporary-table branch from f562673 to 49d6569 Compare April 11, 2019 23:49

arhimondr changed the title ~~[WIP] Allow to change compression codec with a session property~~ Allow to change compression codec with a session property Apr 11, 2019

rschlussel reviewed Apr 12, 2019

View reviewed changes

wenleix reviewed Apr 15, 2019

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/HiveSessionProperties.java Outdated Show resolved Hide resolved

arhimondr commented Apr 17, 2019

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/util/ConfigurationUtils.java Outdated Show resolved Hide resolved

arhimondr force-pushed the change-compression-for-temporary-table branch from 49d6569 to b5b82b6 Compare April 17, 2019 20:24

wenleix reviewed Apr 19, 2019

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/HiveWritableTableHandle.java Show resolved Hide resolved

wenleix reviewed Apr 19, 2019

View reviewed changes

wenleix approved these changes Apr 19, 2019

View reviewed changes

wenleix assigned arhimondr and unassigned wenleix Apr 19, 2019

arhimondr force-pushed the change-compression-for-temporary-table branch from b5b82b6 to 3a5d1f2 Compare April 22, 2019 20:42

arhimondr assigned rschlussel and unassigned arhimondr and rschlussel Apr 22, 2019

arhimondr added 2 commits April 29, 2019 11:49

Add compression_codec session property in Hive connector

a0aeb66

Refactor HiveClientConfig

1a4fa91

Rename hiveCompressionCodec to simply compressionCodec

arhimondr added 3 commits April 29, 2019 11:53

Add temporary_table_compression_codec session property

a4174aa

And hive.temporary-table-compression-codec configuration property

Simplify HiveWriterFactory#getFileExtension

a62bb04

Fix configureCompression method for FileSystemFactory

785a503

arhimondr force-pushed the change-compression-for-temporary-table branch from 3a5d1f2 to 785a503 Compare April 29, 2019 15:57

rschlussel approved these changes Apr 29, 2019

View reviewed changes

wenleix approved these changes Apr 29, 2019

View reviewed changes

arhimondr merged commit 0dbcfe8 into prestodb:master May 1, 2019

arhimondr deleted the change-compression-for-temporary-table branch May 1, 2019 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to change compression codec with a session property #12647

Allow to change compression codec with a session property #12647

arhimondr commented Apr 11, 2019 •

edited by wenleix

Loading

rschlussel Apr 12, 2019

rschlussel Apr 12, 2019

arhimondr Apr 15, 2019

arhimondr Apr 15, 2019

rschlussel Apr 15, 2019

arhimondr Apr 17, 2019

wenleix left a comment •

edited

Loading

arhimondr commented Apr 15, 2019

wenleix commented Apr 15, 2019

arhimondr commented Apr 15, 2019

wenleix commented Apr 15, 2019

wenleix left a comment

wenleix Apr 19, 2019

arhimondr Apr 22, 2019

wenleix left a comment

wenleix Apr 19, 2019

arhimondr Apr 22, 2019

wenleix left a comment

wenleix Apr 19, 2019

arhimondr Apr 22, 2019

arhimondr commented Apr 22, 2019

Allow to change compression codec with a session property #12647

Allow to change compression codec with a session property #12647

Conversation

arhimondr commented Apr 11, 2019 • edited by wenleix Loading

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

wenleix left a comment • edited Loading

Choose a reason for hiding this comment

arhimondr commented Apr 15, 2019

wenleix commented Apr 15, 2019

arhimondr commented Apr 15, 2019

wenleix commented Apr 15, 2019

wenleix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenleix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenleix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arhimondr commented Apr 22, 2019

arhimondr commented Apr 11, 2019 •

edited by wenleix

Loading

wenleix left a comment •

edited

Loading