Uh oh!
There was an error while loading. Please reload this page.
[SPARK-4176][WIP] Support decimal types with precision > 18 in parquet - #6796
[SPARK-4176][WIP] Support decimal types with precision > 18 in parquet#6796rtreffer wants to merge 1 commit into
Conversation
rtreffer
commented
Jun 13, 2015
Note: I came across https://issues.apache.org/jira/browse/SPARK-8342 while testing, it seems like Decimal math is unsafe at the moment. |
There was a problem hiding this comment.
The type of d is (Int, Int). So I think it already contains precision and scale, why to add s?
There was a problem hiding this comment.
It caused a warning on my system. So I thought it would be better to make it explicit.
I could drop that line from this patch, though.
There was a problem hiding this comment.
This should be correct. Scala pattern extractors use tuples if they want to return multiple values.
There was a problem hiding this comment.
As said it was only about a warning, not about correctness. I'll drop this change on the next version, it draws too much attention and is not needed.
32630df to
8f6445cComparemarmbrus
commented
Jun 17, 2015
ok to test |
SparkQA
commented
Jun 17, 2015
Test build #35062 has finished for PR 6796 at commit
|
8f6445c to
7310902CompareSparkQA
commented
Jun 18, 2015
Test build #35115 has finished for PR 6796 at commit
|
marmbrus
commented
Jun 18, 2015
Thanks for working on this! Did a quick pass and it looks pretty good. I'll let @liancheng do a more complete review. Does SPARK-8342 block merging this or can you remove the WIP tag now? |
rtreffer
commented
Jun 18, 2015
The only reason for the WIP is that I have not yet cross-tested the interoperability with e.g. hive. It follows the spec, but I'd like to test it (or have s.o. verify this). |
marmbrus
commented
Jun 18, 2015
Ah cool, it might even be a good idea to check in small files that are created with other systems for these kinds of tests. |
rtreffer
commented
Jun 18, 2015
I was thinking about this: Create a small parquet file with spark, load it with hive, copy it to a new parquet table with hive, read that with spark. If that matches the input -> win. Otherwise -> some more work. PS: SPARK-8342 / SPARK-8359 were only problems during my initial tests. It's a bit harder to test read/write of Decimal if the implementation has bugs. So those are unrelated to this patch, but they might reduce the usefulness of this patch (you can't do reliable math in the ranges you could now load/save) |
marmbrus
commented
Jun 18, 2015
Are you thinking about doing this as part of the test or doing it manually? Right now parquet and its tests have no hive dependencies, which I think is good. But I would definitely like to have a test that reads a file that was written by hive/impala/etc (perhaps created manually and checked in). /cc @liancheng who has also been working on parquet interop. This could also maybe come as a follow-up PR if we want to add the interop tests in one go. |
rtreffer
commented
Jun 18, 2015
Yes, manually. I could add the file I was writing afterwards, sounds like a good idea. |
rtreffer
commented
Jun 19, 2015
Just did a test with hive, I can declare a parquet file written with spark as and it does work. I'm now trying to test the opposite direction plus a test case. I've also dropped the WIP. Doesn't make sense anymore. |
rtreffer
commented
Jun 19, 2015
(hive 1.2.0 and hadoop 2.7.0 without hdfs or a cluster) |
rtreffer
commented
Jun 19, 2015
Ok, it looks like I can't open hive generated parquet files, but it looks more like a type error. Hm, could be that the spark decoder is too strict. There are various ways to encode DECIMAL(30), and it looks like hive chooses fixed_len arrays, while I prefer variable length arrays. Have to double check that. |
rtreffer
commented
Jun 19, 2015
I've pushed the hive generated parquet file and I'll call it a day. I think I'll have to relax the validation of column types for DECIMAL. |
SparkQA
commented
Jun 19, 2015
Test build #35317 has finished for PR 6796 at commit
|
rtreffer
commented
Jun 20, 2015
Ok, I think I'm slowly getting down to the cause.... The relevant class for the job setup is ParquetTableScan (doExecute). I'm not yet sure if this can be fixed on the job setup or on the receiver side. |
rtreffer
commented
Jun 20, 2015
The problematic line is is replaced by I removed that line and the loading of the data works. |
rtreffer
commented
Jun 20, 2015
I've pushed a very early version of a fix. (Literally early, it's nearly 1:00 am. And I'd expect the test build to fail, I'll fix the outstanding issues later today) PS: Loading the hive parquet works now, but I've not yet tested much more. |
There was a problem hiding this comment.
These could be
parquetSchema.filter(_.containsField(name)).map(_.getType(name))
There was a problem hiding this comment.
Ah, yes, an early version had that, I somehow moved to this verbose code O.o
Thanks
Am 21. Juni 2015 02:33:15 MESZ, schrieb Davies Liu notifications@github.com:
toThriftSchemaNames: Boolean = false): ParquetType = {
- val parquetElementTypeBySchema =
These could be
parquetSchema.filter(_.containsField(name)).map(_.getType(name))Reply to this email directly or view it on GitHub:
https://github.com/apache/spark/pull/6796/files#r32889401
There was a problem hiding this comment.
It also performs a type check / conversion. That's why I've removed it. It would look like this
val parquetElementTypeBySchema =
parquetSchema.filter(_.isInstanceOf[ParquetGroupType]).filter(_.containsField(name)).map(_.getType(name))
I would settle on collect, does that look ok?
val parquetElementTypeBySchema = parquetSchema.collect {
case gType : ParquetGroupType if (gType.containsField(name)) => gType.getType(name)
}
SparkQA
commented
Jun 21, 2015
Test build #35373 has finished for PR 6796 at commit
|
There was a problem hiding this comment.
We should int32, int64 if possible, see https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#decimal
DECIMAL can be used to annotate the following types:
int32: for 1 <= precision <= 9
int64: for 1 <= precision <= 18; precision <= 10 will produce a warning
fixed_len_byte_array: precision is limited by the array size. Length n can store <= floor(log_10(2^(8*n - 1) - 1)) base-10 digits
binary: precision is not limited, but is required. The minimum number of bytes to store the unscaled value should be used.
There was a problem hiding this comment.
What would we gain by encoding it that way?
We use a minimal length fixed byte array which should provide a similar compact encoding. (DECIMAL(9) should end up as 4 bytes, and smaller decimal values should take even less space)
Decoding is a different story, though.
PS: I was focusing on DECIMAL with precision >=19. Shouldn't small decimal handling be a new ticket?
Am 21. Juni 2015 02:41:44 MESZ, schrieb Davies Liu notifications@github.com:
@@ -229,11 +231,15 @@ private[parquet] object ParquetTypesConverter
extends Logging {
case LongType =>
Some(ParquetTypeInfo(ParquetPrimitiveTypeName.INT64))
case TimestampType =>
Some(ParquetTypeInfo(ParquetPrimitiveTypeName.INT96))
case DecimalType.Fixed(precision, scale) if precision <= 18 =>
in a Long// TODO: for now, our writer only supports decimals that fitSome(ParquetTypeInfo(ParquetPrimitiveTypeName.FIXED_LEN_BYTE_ARRAY,
We should int32, int64 if possible, see
https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#decimalDECIMAL can be used to annotate the following types: int32: for 1 <= precision <= 9 int64: for 1 <= precision <= 18; precision <= 10 will produce a warning fixed_len_byte_array: precision is limited by the array size. Length n can store <= floor(log_10(2^(8*n - 1) - 1)) base-10 digits binary: precision is not limited, but is required. The minimum number of bytes to store the unscaled value should be used.Reply to this email directly or view it on GitHub:
https://github.com/apache/spark/pull/6796/files#r32889441
Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.
There was a problem hiding this comment.
Using int32 and int64 makes encoding and decoding faster since they don't introduce boxing costs. But I agree that should be made in another PR.
464d24e to
f973b58CompareSparkQA
commented
Jun 21, 2015
Test build #35414 has finished for PR 6796 at commit
|
f973b58 to
8ff6603CompareSparkQA
commented
Jun 23, 2015
Test build #35545 has finished for PR 6796 at commit
|
rtreffer
commented
Jun 29, 2015
Hi @liancheng, thank you for the thorough review, will push a reworked version soon. Everything sounds reasonable :-) With "private" Settings I meant that I can't change the setting in the shell because it's marked as "isPublic = false" in https://github.com/liancheng/spark/blob/2a2062d3f530ecd26e75b306aee42761d67d8724/sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala#L273 I'm not sure if that's intended. |
liancheng
commented
Jun 29, 2015
@rtreffer Yeah, it's intended. As explained above, this feature flag must be set to |
liancheng
commented
Jul 6, 2015
Hey @rtreffer, just want to make sure whether you are still working on this? I'm asking because I just opened #7231 to refactor Parquet read path for interoperability and backwards-compatibility, which also touches the decimal parts. I believe the new |
rtreffer
commented
Jul 7, 2015
Hi @liancheng, I'm rebasing on you PR right now. I can work for ~1-2h / day on this PR so feel free to take over the PR if this blocks anything. |
5fe321e to
e6dad45CompareSparkQA
commented
Jul 7, 2015
Test build #36702 has finished for PR 6796 at commit
|
e6dad45 to
7a57c16Comparertreffer
commented
Jul 7, 2015
The writeDecimal method is rather ugly, and the write path needs to know if we follow parquet style or not as this implies a different encoding (addInteger / addLong). |
SparkQA
commented
Jul 7, 2015
Test build #36703 has finished for PR 6796 at commit
|
7a57c16 to
1152721CompareSparkQA
commented
Jul 12, 2015
Test build #37097 has finished for PR 6796 at commit
|
1152721 to
3e30bdfCompareSparkQA
commented
Jul 12, 2015
Test build #37099 has finished for PR 6796 at commit
|
3e30bdf to
c8d4d6cCompareSparkQA
commented
Jul 13, 2015
Test build #37130 has finished for PR 6796 at commit
|
SparkQA
commented
Jul 13, 2015
Test build #1055 has finished for PR 6796 at commit
|
c8d4d6c to
1703c26CompareSparkQA
commented
Jul 13, 2015
Test build #37143 has finished for PR 6796 at commit
|
06d337a to
83ca029Compare…quets fixed_byte_array Parquet defines multiple ways to store decimals. This patch enables the reading of all variations as well as writing decimals in the smallest fixed-length container possible (INT32, INT64, FIXED_LEN_BYTE_ARRAY).
83ca029 to
1dad677CompareSparkQA
commented
Jul 13, 2015
Test build #37146 has finished for PR 6796 at commit
|
SparkQA
commented
Jul 13, 2015
Test build #37147 has finished for PR 6796 at commit
|
liancheng
commented
Jul 18, 2015
rtreffer
commented
Jul 18, 2015
@liancheng sure, I just wasn't sure if it should be closed :-) |
This PR is based on #6796 authored by rtreffer. To support large decimal precisions (> 18), we do the following things in this PR: 1. Making `CatalystSchemaConverter` support large decimal precision Decimal types with large precision are always converted to fixed-length byte array. 2. Making `CatalystRowConverter` support reading decimal values with large precision When the precision is > 18, constructs `Decimal` values with an unscaled `BigInteger` rather than an unscaled `Long`. 3. Making `RowWriteSupport` support writing decimal values with large precision In this PR we always write decimals as fixed-length byte array, because Parquet write path hasn't been refactored to conform Parquet format spec (see SPARK-6774 & SPARK-8848). Two follow-up tasks should be done in future PRs: - [ ] Writing decimals as `INT32`, `INT64` when possible while fixing SPARK-8848 - [ ] Adding compatibility tests as part of SPARK-5463 Author: Cheng Lian <lian@databricks.com> Closes#7455 from liancheng/spark-4176 and squashes the following commits: a543d10 [Cheng Lian] Fixes errors introduced while rebasing 9e31cdf [Cheng Lian] Supports decimals with precision > 18 for Parquet
This is my current WIP on SPARK-4176. It should be compatible with other implementations of parquet.
https://github.com/Parquet/parquet-format/blob/master/LogicalTypes.md#decimal
This is the default encoding on bigint. It should thus be compatible with other implementations, although it would be great if s.o. could test this.
I've tested this locally with powers of 2 up to 2^200 in the spark shell, without errors but
Code I've used for (local) testing (on spark shell):