PARQUET-545: Improve API to support decimal type by majetideepak · Pull Request #65 · apache/parquet-cpp

majetideepak · 2016-02-25T17:35:03Z

This PR also

Improves scanner coverage.
Implements PARQUET-546
Adds tests for Types to string

majetideepak · 2016-02-25T23:06:20Z

@wesm I don't see much to do for DECIMAL types except to check that the scale and precision are set properly in the ColumnDescriptor. I modified the ColumnDescriptor and I am yet to add some test cases.
Do you have other thoughts here?
I also implemented PARQUET-546. Can you have a look ? I will add test cases if you are okay with the new API.
Thanks!

majetideepak · 2016-02-25T23:07:42Z

src/parquet/encodings/plain-encoding-test.cc

 TEST(FLBAEncodeDecode, TestEncodeDecode) {
  schema::NodePtr node;
-  node = schema::PrimitiveNode::MakeFLBA("name", Repetition::OPTIONAL,
-      flba_length, LogicalType::UTF8);


We should have a single API to create Primitive nodes.

majetideepak · 2016-02-25T23:38:02Z

I just looked at parquet-mr and it looks like they check if the length precision and scale validate the decimal type. I will add this check.

wesm · 2016-02-26T04:31:11Z

I commented on https://issues.apache.org/jira/browse/PARQUET-545. Impala has a lot of decimal-related code, but it is mostly used for computations in the runtime. I'm not sure if having container types for decimal data (and stuff like coercing to/from double -- that would be useful IMHO) would be helpful overall. Being able to print decimals would be nice. It's a can of worms though

majetideepak · 2016-02-26T16:53:24Z

@wesm I agree. I just commented on https://issues.apache.org/jira/browse/PARQUET-545 too. In this patch, I want to extend the ColumnDescriptor API to be able to extract the scale and precision values required to interpret decimals. We can use the Impala Decimal code at the Scanner level in a later patch.

majetideepak · 2016-02-26T16:54:14Z

src/parquet/schema/descriptor.h


  bool is_required() const {
-    return max_definition_level_ == 0;
+    return primitive_node_->is_required();


We should be using the Type values set in the PrimitiveNode.

majetideepak · 2016-02-26T17:23:24Z

@wesm I feel this patch is important for the release as well. Can you please give feedback. I will rebase this off #64.

wesm · 2016-02-26T17:42:25Z

src/parquet/schema/descriptor.h

  bool is_repeated() const {
-    return max_repetition_level_ > 0;
+    return primitive_node_->is_repeated();
  }


These changes have fundamentally changed the nature of their results -- they can't be used like they were in the scanner any more, because you could have required types with definition level > 0:

optional group bag repeated group list required int32 item

I suggest using the max repetition/definition levels in the scanner to avoid this issue

makes sense. I did not think about the groups. Will revert this.

Since this didn't cause test failures, it may be worth trying to add a failing test case

We don't have any groups tests yet. This code works for all flat schemas.

I will add some GroupNode tests in the descriptor tests

majetideepak · 2016-02-26T21:35:37Z

src/parquet/schema/schema-descriptor-test.cc

  ASSERT_THROW(descr_.Init(node), ParquetException);
 }

+TEST_F(TestSchemaDescriptor, DescriptorRepetitionValues) {


Tests for repetition values of schemas with GroupNodes

majetideepak · 2016-02-26T21:55:10Z

The changes are complete. I will wait for the test coverage report.

wesm · 2016-02-26T21:57:00Z

Cool I can review in a little bit

majetideepak · 2016-02-26T22:12:25Z

src/parquet/column/scanner-test.cc

+    reinterpret_cast<TypedScanner<FLBAType::type_num>* >(scanner_.get());
+  ASSERT_EQ(10, scanner->descr()->type_precision());
+  ASSERT_EQ(2, scanner->descr()->type_scale());
+  ASSERT_EQ(FLBA_LENGTH, scanner->descr()->type_length());


Test for ColumnDescriptor API extensions in this patch.

These should probably go in their own test case.

wesm · 2016-02-26T23:50:07Z

Looks like we're blocked on Travis builds again for the next few hours. Let me know on these relatively minor issues and I'll look again a bit later. I'm going to close out the remaining clean-up patches as quickly as I can

wesm · 2016-02-26T23:51:41Z

Also, the PR title needs to be changed to PARQUET-545

majetideepak · 2016-02-27T03:06:35Z

I am making these changes now.

wesm · 2016-02-27T22:01:00Z

+1, thank you!

majetideepak · 2016-02-28T00:15:54Z

@wesm Thanks!
On a separate note, will it help if we test the column reader and scanner for Dictionary pages too ?

wesm · 2016-02-28T01:34:31Z

Yes, that would be a good idea -- I only tested the lowest tier of encoding. Can you open a JIRA?

wesm · 2016-02-28T01:35:00Z

This issue title needs to start with PARQUET-545: (capital letters) or the merge tool will fail

majetideepak · 2016-02-28T01:53:50Z

https://issues.apache.org/jira/browse/PARQUET-549

julienledem · 2016-02-29T18:03:20Z

+1

majetideepak force-pushed the PARQUET-545 branch from bff1f4b to 844a3ab Compare February 25, 2016 22:09

majetideepak reviewed Feb 25, 2016
View reviewed changes

majetideepak changed the title ~~Parquet 545: (WIP) Support for decimals~~ Parquet 545: Improve API to support decimal type Feb 26, 2016

majetideepak reviewed Feb 26, 2016
View reviewed changes

wesm reviewed Feb 26, 2016
View reviewed changes

Deepak Majeti added 10 commits February 26, 2016 13:57

Improve Scanner Coverage

938ded5

Extend Column Descriptor to include Decimal scale and precision

bdbcdd7

PARQUET-546

a346ca5

modified scanner and column descriptor

8a043c0

remove unnecessary checks

addf7f6

restored some necessary checks

656942f

Added PrimitiveNode Tests

a643bf4

Tests for PARQUET-545

6ce10ce

Improve coverage for types test

cab06d8

added type to string tests

0f0e559

majetideepak force-pushed the PARQUET-545 branch from e7b7ad2 to 0f0e559 Compare February 26, 2016 20:36

Deepak Majeti added 2 commits February 26, 2016 16:17

Added tests for GroupNodes

1eed0de

modified scanner test to remove redundant code

6b450ab

majetideepak reviewed Feb 26, 2016
View reviewed changes

majetideepak changed the title ~~Parquet 545: Improve API to support decimal type~~ Parquet-545: Improve API to support decimal type Feb 27, 2016

Deepak Majeti added 2 commits February 26, 2016 22:25

removed ColumnDescriptor Repetition API

6a18c98

improved schema type coverage

c5f6d85

removed little redundant code

df28ccc

majetideepak changed the title ~~Parquet-545: Improve API to support decimal type~~ PARQUET-545: Improve API to support decimal type Feb 28, 2016

asfgit closed this in ebb45b1 Feb 29, 2016

majetideepak deleted the PARQUET-545 branch March 3, 2016 15:00

Conversation

majetideepak commented Feb 25, 2016

Uh oh!

majetideepak commented Feb 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majetideepak commented Feb 25, 2016

Uh oh!

wesm commented Feb 26, 2016

Uh oh!

majetideepak commented Feb 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majetideepak commented Feb 26, 2016

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!

majetideepak commented Feb 26, 2016

Uh oh!

wesm commented Feb 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Feb 26, 2016

Uh oh!

wesm commented Feb 26, 2016

Uh oh!

majetideepak commented Feb 27, 2016

Uh oh!

wesm commented Feb 27, 2016

Uh oh!

majetideepak commented Feb 28, 2016

Uh oh!

wesm commented Feb 28, 2016

Uh oh!

wesm commented Feb 28, 2016

Uh oh!

majetideepak commented Feb 28, 2016

Uh oh!

julienledem commented Feb 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants