[fix](iceberg)Bring field_id with parquet files And fix map type's key optional #44470

wuwenchi · 2024-11-22T07:38:25Z

What problem does this PR solve?

Column IDs are required to be stored as field IDs on the parquet schema.
ref: https://iceberg.apache.org/spec/?h=field+id#parquet
So, we should add field ids.
For MapType, its key is always required.

Release note

None

Check List (For Author)

Test
- Regression test
- Unit Test
- Manual test (add detailed scripts or steps below)
- No need to test or manual test. Explain why:
  - This is a refactor/code format and no logic has been changed.
  - Previous test can cover this change.
  - No code files have been changed.
  - Other reason
Behavior changed:
- No.
- Yes.
Does this need documentation?
- No.
- Yes.

Check List (For Reviewer who merge this PR)

Confirm the release note
Confirm test cases
Confirm document
Add branch pick label

doris-robot · 2024-11-22T07:38:30Z

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

What problem was fixed (it's best to include specific error reporting information). How it was fixed.
Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
What features were added. Why was this function added?
Which code was refactored and why was this part of the code refactored?
Which functions were optimized and what is the difference before and after the optimization?

wuwenchi · 2024-11-22T07:39:17Z

run buildall

wuwenchi · 2024-11-22T07:40:42Z

run buildall

github-actions

clang-tidy made some suggestions

github-actions · 2024-11-22T07:43:23Z

be/src/vec/exec/format/table/iceberg/arrow_schema_util.cpp

+namespace doris {
+namespace iceberg {


warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]

Suggested change

namespace doris {

namespace iceberg {

namespace doris::iceberg {

be/src/vec/exec/format/table/iceberg/arrow_schema_util.cpp:132:

- } // namespace iceberg - } // namespace doris + } // namespace doris

github-actions · 2024-11-22T07:43:24Z

be/src/vec/exec/format/table/iceberg/arrow_schema_util.cpp

+    return Status::OK();
+}
+
+Status ArrowSchemaUtil::ConvertTo(const iceberg::NestedField& field,


warning: function 'ConvertTo' exceeds recommended size/complexity thresholds [readability-function-size]

Status ArrowSchemaUtil::ConvertTo(const iceberg::NestedField& field, ^

Additional context

be/src/vec/exec/format/table/iceberg/arrow_schema_util.cpp:39: 89 lines including whitespace and comments (threshold 80)

Status ArrowSchemaUtil::ConvertTo(const iceberg::NestedField& field, ^

github-actions · 2024-11-22T07:43:24Z

be/src/vec/exec/format/table/iceberg/arrow_schema_util.cpp

+        break;
+
+    case iceberg::TypeID::DECIMAL: {
+        DecimalType* dt = dynamic_cast<DecimalType*>(field.field_type());


warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]

Suggested change

DecimalType* dt = dynamic_cast<DecimalType*>(field.field_type());

auto* dt = dynamic_cast<DecimalType*>(field.field_type());

github-actions · 2024-11-22T07:43:24Z

be/src/vec/exec/format/table/iceberg/arrow_schema_util.h

+// #include <sys/types.h>
+// #include <unistd.h>
+
+#include <arrow/type.h>


warning: 'arrow/type.h' file not found [clang-diagnostic-error]

#include <arrow/type.h> ^

github-actions · 2024-11-22T07:43:24Z

be/src/vec/exec/format/table/iceberg/arrow_schema_util.h

+
+namespace doris {


warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]

Suggested change

namespace doris {

namespace doris::iceberg {

be/src/vec/exec/format/table/iceberg/arrow_schema_util.h:47:

- } // namespace iceberg - } // namespace doris + } // namespace doris

github-actions · 2024-11-22T07:43:24Z

be/src/vec/exec/format/table/iceberg/types.h

@@ -265,6 +265,10 @@ class DecimalType : public PrimitiveType {
        ss << "decimal(" << precision << ", " << scale << ")";
        return ss.str();
    }
+
+    int get_precision() { return precision; }


warning: method 'get_precision' can be made const [readability-make-member-function-const]

Suggested change

int get_precision() { return precision; }

int get_precision() const { return precision; }

github-actions · 2024-11-22T07:43:24Z

be/src/vec/exec/format/table/iceberg/types.h

+
+    int get_precision() { return precision; }
+
+    int get_scale() { return scale; }


warning: method 'get_scale' can be made const [readability-make-member-function-const]

Suggested change

int get_scale() { return scale; }

int get_scale() const { return scale; }

github-actions · 2024-11-22T07:43:24Z

be/test/vec/exec/format/table/iceberg/arrow_schema_util_test.cpp

+namespace doris {
+namespace iceberg {


warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]

Suggested change

namespace doris {

namespace iceberg {

namespace doris::iceberg {

be/test/vec/exec/format/table/iceberg/arrow_schema_util_test.cpp:219:

- } // namespace iceberg - } // namespace doris + } // namespace doris

github-actions

clang-tidy made some suggestions

github-actions · 2024-11-22T07:44:55Z

be/src/vec/exec/format/table/iceberg/arrow_schema_util.h

+namespace doris {
+namespace iceberg {


warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]

Suggested change

namespace doris {

namespace iceberg {

namespace doris::iceberg {

be/src/vec/exec/format/table/iceberg/arrow_schema_util.h:48:

- } // namespace iceberg - } // namespace doris + } // namespace doris

doris-robot · 2024-11-22T08:09:30Z

TeamCity be ut coverage result:
Function Coverage: 38.34% (9989/26052)
Line Coverage: 29.44% (83592/283893)
Region Coverage: 28.60% (43007/150388)
Branch Coverage: 25.19% (21863/86782)
Coverage Report: http://coverage.selectdb-in.cc/coverage/725442243802c51ace1a691d0fb4c77d116ae90c_725442243802c51ace1a691d0fb4c77d116ae90c/report/index.html

wuwenchi · 2024-11-22T08:53:41Z

run buildall

github-actions

clang-tidy made some suggestions

github-actions · 2024-11-22T08:58:11Z

be/src/vec/exec/format/table/iceberg/arrow_schema_util.cpp

+    return Status::OK();
+}
+
+Status ArrowSchemaUtil::convert_to(const iceberg::NestedField& field,


warning: function 'convert_to' exceeds recommended size/complexity thresholds [readability-function-size]

Status ArrowSchemaUtil::convert_to(const iceberg::NestedField& field, ^

Additional context

be/src/vec/exec/format/table/iceberg/arrow_schema_util.cpp:39: 89 lines including whitespace and comments (threshold 80)

Status ArrowSchemaUtil::convert_to(const iceberg::NestedField& field, ^

github-actions

clang-tidy made some suggestions

github-actions · 2024-11-22T09:04:54Z

be/src/vec/exec/format/table/iceberg/arrow_schema_util.cpp

+        break;
+
+    case iceberg::TypeID::DECIMAL: {
+        auto dt = dynamic_cast<DecimalType*>(field.field_type());


warning: 'auto dt' can be declared as 'auto *dt' [readability-qualified-auto]

Suggested change

auto dt = dynamic_cast<DecimalType*>(field.field_type());

auto *dt = dynamic_cast<DecimalType*>(field.field_type());

github-actions · 2024-11-22T09:04:54Z

be/src/vec/exec/format/table/iceberg/arrow_schema_util.h

+namespace doris {
+namespace iceberg {


warning: nested namespaces can be concatenated [modernize-concat-nested-namespaces]

Suggested change

namespace doris {

namespace iceberg {

namespace doris::iceberg {

be/src/vec/exec/format/table/iceberg/arrow_schema_util.h:43:

- } // namespace iceberg - } // namespace doris + } // namespace doris

doris-robot · 2024-11-22T09:26:02Z

TeamCity be ut coverage result:
Function Coverage: 38.34% (9987/26051)
Line Coverage: 29.44% (83570/283903)
Region Coverage: 28.59% (42994/150387)
Branch Coverage: 25.19% (21858/86782)
Coverage Report: http://coverage.selectdb-in.cc/coverage/64a2106ab2d2aa668ede3f1e79b059efd4e3d085_64a2106ab2d2aa668ede3f1e79b059efd4e3d085/report/index.html

wuwenchi · 2024-11-22T09:49:49Z

run buildall

doris-robot · 2024-11-22T10:17:33Z

TeamCity be ut coverage result:
Function Coverage: 38.33% (9986/26053)
Line Coverage: 29.45% (83600/283918)
Region Coverage: 28.59% (42993/150394)
Branch Coverage: 25.18% (21856/86786)
Coverage Report: http://coverage.selectdb-in.cc/coverage/5732eb1cfc73f5a8b2a0daf40e12be5cb5eb7944_5732eb1cfc73f5a8b2a0daf40e12be5cb5eb7944/report/index.html

wuwenchi · 2024-11-25T12:48:26Z

run p0

wuwenchi · 2024-11-25T12:48:32Z

run external

wuwenchi · 2024-11-26T05:06:41Z

run buildall

doris-robot · 2024-11-26T06:24:56Z

TeamCity be ut coverage result:
Function Coverage: 38.40% (9989/26011)
Line Coverage: 29.48% (83610/283568)
Region Coverage: 28.62% (43026/150314)
Branch Coverage: 25.23% (21863/86664)
Coverage Report: http://coverage.selectdb-in.cc/coverage/f8085c4bb9f6d8199b0e27e99cb6da6658f7aae5_f8085c4bb9f6d8199b0e27e99cb6da6658f7aae5/report/index.html

wuwenchi · 2024-11-26T09:52:56Z

run buildall

doris-robot · 2024-11-26T11:04:42Z

TeamCity be ut coverage result:
Function Coverage: 38.38% (9985/26017)
Line Coverage: 29.46% (83542/283593)
Region Coverage: 28.61% (43003/150318)
Branch Coverage: 25.22% (21857/86662)
Coverage Report: http://coverage.selectdb-in.cc/coverage/5d0a5a90d7209f69967ca738f9f61e4a26ea8c0e_5d0a5a90d7209f69967ca738f9f61e4a26ea8c0e/report/index.html

wuwenchi · 2024-11-27T02:52:01Z

run p0

morningman

LGTM

github-actions · 2024-11-27T13:33:39Z

PR approved by at least one committer and no changes requested.

github-actions · 2024-11-27T13:33:41Z

PR approved by anyone and no changes requested.

wuwenchi · 2024-11-28T04:22:57Z

run buildall

doris-robot · 2024-11-28T05:47:26Z

TeamCity be ut coverage result:
Function Coverage: 38.35% (9983/26029)
Line Coverage: 29.44% (83570/283858)
Region Coverage: 28.59% (43012/150470)
Branch Coverage: 25.21% (21861/86714)
Coverage Report: http://coverage.selectdb-in.cc/coverage/ab32e2f29df8f75f5e86ab0eb59b94cf14f743d1_ab32e2f29df8f75f5e86ab0eb59b94cf14f743d1/report/index.html

github-actions · 2024-11-29T14:35:38Z

PR approved by at least one committer and no changes requested.

…y optional (#44470) ### What problem does this PR solve? 1. Column IDs are required to be stored as [field IDs](http://github.com/apache/parquet-format/blob/40699d05bd24181de6b1457babbee2c16dce3803/src/main/thrift/parquet.thrift#L459) on the parquet schema. ref: https://iceberg.apache.org/spec/?h=field+id#parquet So, we should add field ids. 2. For `MapType`, its key is always required.

…ap type's key optional #44470 (#44828) Cherry-picked from #44470 Co-authored-by: wuwenchi <[email protected]>

…ap type's key optional #44470 (#44827) Cherry-picked from #44470 Co-authored-by: wuwenchi <[email protected]>

github-actions bot reviewed Nov 22, 2024

View reviewed changes

morningman added dev/2.1.x dev/3.0.x labels Nov 25, 2024

wuwenchi added 5 commits November 26, 2024 13:05

fix

596ac8f

format

fcdf078

fix

a991552

format

14fbc8b

format

f8085c4

wuwenchi force-pushed the fix-field-id branch from 5732eb1 to f8085c4 Compare November 26, 2024 05:05

fix init

5d0a5a9

morningman previously approved these changes Nov 27, 2024

View reviewed changes

github-actions bot added the approved Indicates a PR has been approved by one committer. label Nov 27, 2024

github-actions bot added the reviewed label Nov 27, 2024

add test

ab32e2f

wuwenchi dismissed morningman’s stale review via ab32e2f November 28, 2024 04:22

github-actions bot removed the approved Indicates a PR has been approved by one committer. label Nov 28, 2024

morningman approved these changes Nov 29, 2024

View reviewed changes

github-actions bot added the approved Indicates a PR has been approved by one committer. label Nov 29, 2024

CalvinKirs approved these changes Dec 1, 2024

View reviewed changes

morningman merged commit 9ca8048 into apache:master Dec 1, 2024
23 of 26 checks passed

github-actions bot mentioned this pull request Dec 1, 2024

branch-3.0: [fix](iceberg)Bring field_id with parquet files And fix map type's key optional #44470 #44827

Merged

github-actions bot mentioned this pull request Dec 1, 2024

branch-2.1: [fix](iceberg)Bring field_id with parquet files And fix map type's key optional #44470 #44828

Merged

yiguolei pushed a commit that referenced this pull request Dec 2, 2024

branch-2.1: [fix](iceberg)Bring field_id with parquet files And fix m…

5f952cf

…ap type's key optional #44470 (#44828) Cherry-picked from #44470 Co-authored-by: wuwenchi <[email protected]>

yiguolei added dev/2.1.8-merged and removed dev/2.1.x labels Dec 2, 2024

morningman pushed a commit that referenced this pull request Dec 2, 2024

branch-3.0: [fix](iceberg)Bring field_id with parquet files And fix m…

47fbbfa

…ap type's key optional #44470 (#44827) Cherry-picked from #44470 Co-authored-by: wuwenchi <[email protected]>

morningman added dev/3.0.4-merged and removed dev/3.0.x labels Dec 2, 2024

yiguolei mentioned this pull request Jan 19, 2025

Release Note 2.1.8 #47198

Open

	namespace doris {
	namespace iceberg {
	namespace doris::iceberg {

	DecimalType* dt = dynamic_cast<DecimalType*>(field.field_type());
	auto* dt = dynamic_cast<DecimalType*>(field.field_type());

	int get_precision() { return precision; }
	int get_precision() const { return precision; }


		int get_precision() { return precision; }

		int get_scale() { return scale; }

	int get_scale() { return scale; }
	int get_scale() const { return scale; }

	auto dt = dynamic_cast<DecimalType*>(field.field_type());
	auto dt = dynamic_cast<DecimalType>(field.field_type());

[fix](iceberg)Bring field_id with parquet files And fix map type's key optional #44470

[fix](iceberg)Bring field_id with parquet files And fix map type's key optional #44470

Conversation

wuwenchi commented Nov 22, 2024 • edited Loading

What problem does this PR solve?

Release note

Check List (For Author)

Check List (For Reviewer who merge this PR)

doris-robot commented Nov 22, 2024

wuwenchi commented Nov 22, 2024

wuwenchi commented Nov 22, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Nov 22, 2024

Choose a reason for hiding this comment

github-actions bot Nov 22, 2024

Choose a reason for hiding this comment

github-actions bot Nov 22, 2024

Choose a reason for hiding this comment

github-actions bot Nov 22, 2024

Choose a reason for hiding this comment

github-actions bot Nov 22, 2024

Choose a reason for hiding this comment

github-actions bot Nov 22, 2024

Choose a reason for hiding this comment

github-actions bot Nov 22, 2024

Choose a reason for hiding this comment

github-actions bot Nov 22, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Nov 22, 2024

Choose a reason for hiding this comment

doris-robot commented Nov 22, 2024

wuwenchi commented Nov 22, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Nov 22, 2024

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Nov 22, 2024

Choose a reason for hiding this comment

github-actions bot Nov 22, 2024

Choose a reason for hiding this comment

doris-robot commented Nov 22, 2024

wuwenchi commented Nov 22, 2024

doris-robot commented Nov 22, 2024

wuwenchi commented Nov 25, 2024

wuwenchi commented Nov 25, 2024

wuwenchi commented Nov 26, 2024

doris-robot commented Nov 26, 2024

wuwenchi commented Nov 26, 2024

doris-robot commented Nov 26, 2024

wuwenchi commented Nov 27, 2024

morningman left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 27, 2024

github-actions bot commented Nov 27, 2024

wuwenchi commented Nov 28, 2024

doris-robot commented Nov 28, 2024

github-actions bot commented Nov 29, 2024

wuwenchi commented Nov 22, 2024 •

edited

Loading