You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Quanlong Huang (Code Review)" <ge...@cloudera.org> on 2024/12/29 07:35:53 UTC
[Impala-ASF-CR] IMPALA-13305: Better thrift compatability checks based on pyparsing
Quanlong Huang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/22264
Change subject: IMPALA-13305: Better thrift compatability checks based on pyparsing
......................................................................
IMPALA-13305: Better thrift compatability checks based on pyparsing
There are some false positive warnings reported by
critique-gerrit-review.py when adding a new thrift struct that has
required fields. This patch leverages pyparsing to analyze the
thrift file changes. So we can identify whether the new required field
is added in an existing struct.
thrift_parser.py adds a simple thrift gramma parser to parse a thrift
file into an AST. It basically consists of pyparsing.ParseResults and
some customized classes to inject the line number, i.e.
thrift_parser.ThriftField and thrift_parser.ThriftEnumItem.
The thrift_parser is used to parse the current version of a thrift file
and the old version of it before the commit. critique-gerrit-review.py
then compares the structs and enums to report these warnings:
- A required field is deleted in an existing struct.
- A new required field is added in an existing struct.
- An existing field is renamed.
- The qualifier (required/optional) of a field is changed.
- The type of a field is changed.
- An enum item is removed.
- Enum items are reordered.
Only thrift files used in both catalogd and impalad are checked. This is
the same as the current version. We can further improve this by
analyzing all RPCs used between impalad and catalogd to get all thrift
struct/enums used in them.
Warning examples for commit e48af8c04:
"common/thrift/StatestoreService.thrift": [
{
"message": "Renaming field 'sequence' to 'catalogd_version' in TUpdateCatalogdRequest might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 345,
"side": "REVISION"
}
]
Warning examples for commit 595212b4e:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Adding a required field 'type' in TIcebergPartitionField might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 612,
"side": "REVISION"
}
]
Warning examples for commit c57921225:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Renaming field 'partition_id' to 'spec_id' in TIcebergPartitionSpec might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 606,
"side": "REVISION"
}
],
"common/thrift/CatalogService.thrift": [
{
"message": "Changing field 'iceberg_data_files_fb' from required to optional in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 215,
"side": "REVISION"
},
{
"message": "Adding a required field 'operation' in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 209,
"side": "REVISION"
}
],
"common/thrift/Query.thrift": [
{
"message": "Renaming field 'spec_id' to 'iceberg_params' in TFinalizeParams might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 876,
"side": "REVISION"
}
]
Warning example for commit 2b2cf8d96:
"common/thrift/CatalogService.thrift": [
{
"message": "Enum item FUNCTION_NOT_FOUND=3 changed to TABLE_NOT_LOADED=3 in CatalogLookupStatus. This might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 381,
"side": "REVISION"
}
]
Warning example for commit c01efd096:
"common/thrift/JniCatalog.thrift": [
{
"message": "Removing the enum item TAlterTableType.SET_OWNER=15 might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 107,
"side": "REVISION"
}
]
Tests
- Verified the script with all(1260) commits of common/thrift.
Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
---
M bin/jenkins/critique-gerrit-review.py
A bin/jenkins/thrift_parser.py
2 files changed, 305 insertions(+), 94 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/22264/1
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
[Impala-ASF-CR] IMPALA-13305: Better thrift compatability checks based on pyparsing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatability checks based on pyparsing
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17771/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sun, 29 Dec 2024 11:23:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatability checks based on pyparsing
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatability checks based on pyparsing
......................................................................
Patch Set 3:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG@15
PS3, Line 15: gramma
nit: grammar
http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG@20
PS3, Line 20: The thrift_parser is used to
nit: Import thrift_parser to
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py@53
PS3, Line 53: PYPARSING_VERSION = "3.2.0"
Do we need to reconcile this version with another one defined and used here?
infra/python/deps/requirements.txt:41:pyparsing == 2.0.3
tests/comparison/db_connection.py:37:from pyparsing import (
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py@379
PS3, Line 379: curr_structs, curr_enums = extract_thrift_defs(revision, curr_file)
: old_structs, old_enums = extract_thrift_defs(base_revision, curr_file)
: compare_thrift_structs(curr_file, old_structs, curr_structs, comments)
: compare_thrift_enums(curr_file, old_enums, curr_enums, comments)
What do you think of moving function definitions of these into thrift_parser.py?
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py
File bin/jenkins/thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@19
PS3, Line 19: Developed based on
nit: Developed based on example at:
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@20
PS3, Line 20: https://github.com/pyparsing/pyparsing/blob/master/examples/protobuf_parser.py
Consider using permalink:
https://github.com/pyparsing/pyparsing/blob/c4cf4a5/examples/protobuf_parser.py
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@54
PS3, Line 54: https://github.com/apache/thrift/blob/master/compiler/cpp/src/thrift/thriftl.ll
Consider using permalink:
https://github.com/apache/thrift/blob/154d154/compiler/cpp/src/thrift/thriftl.ll
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@186
PS3, Line 186: # thrift_dir = "/home/quanlong/workspace/Impala/common/thrift"
nit:
# thrift_dir = os.environ["IMPALA_HOME"] + "/common/thrift"
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@195
PS3, Line 195: res.pprint()
Can you write a simple test to check output of this script? Maybe put it under tests/infra/?
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Dec 2024 16:28:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Riza Suminto, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22264
to look at the new patch set (#4).
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
IMPALA-13305: Better thrift compatibility checks based on pyparsing
There are some false positive warnings reported by
critique-gerrit-review.py when adding a new thrift struct that has
required fields. This patch leverages pyparsing to analyze the
thrift file changes. So we can identify whether the new required field
is added in an existing struct.
thrift_parser.py adds a simple thrift grammar parser to parse a thrift
file into an AST. It basically consists of pyparsing.ParseResults and
some customized classes to inject the line number, i.e.
thrift_parser.ThriftField and thrift_parser.ThriftEnumItem.
Import thrift_parser to parse the current version of a thrift file and
the old version of it before the commit. critique-gerrit-review.py
then compares the structs and enums to report these warnings:
- A required field is deleted in an existing struct.
- A new required field is added in an existing struct.
- An existing field is renamed.
- The qualifier (required/optional) of a field is changed.
- The type of a field is changed.
- An enum item is removed.
- Enum items are reordered.
Only thrift files used in both catalogd and impalad are checked. This is
the same as the current version. We can further improve this by
analyzing all RPCs used between impalad and catalogd to get all thrift
struct/enums used in them.
Warning examples for commit e48af8c04:
"common/thrift/StatestoreService.thrift": [
{
"message": "Renaming field 'sequence' to 'catalogd_version' in TUpdateCatalogdRequest might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 345,
"side": "REVISION"
}
]
Warning examples for commit 595212b4e:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Adding a required field 'type' in TIcebergPartitionField might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 612,
"side": "REVISION"
}
]
Warning examples for commit c57921225:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Renaming field 'partition_id' to 'spec_id' in TIcebergPartitionSpec might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 606,
"side": "REVISION"
}
],
"common/thrift/CatalogService.thrift": [
{
"message": "Changing field 'iceberg_data_files_fb' from required to optional in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 215,
"side": "REVISION"
},
{
"message": "Adding a required field 'operation' in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 209,
"side": "REVISION"
}
],
"common/thrift/Query.thrift": [
{
"message": "Renaming field 'spec_id' to 'iceberg_params' in TFinalizeParams might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 876,
"side": "REVISION"
}
]
Warning example for commit 2b2cf8d96:
"common/thrift/CatalogService.thrift": [
{
"message": "Enum item FUNCTION_NOT_FOUND=3 changed to TABLE_NOT_LOADED=3 in CatalogLookupStatus. This might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 381,
"side": "REVISION"
}
]
Warning example for commit c01efd096:
"common/thrift/JniCatalog.thrift": [
{
"message": "Removing the enum item TAlterTableType.SET_OWNER=15 might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 107,
"side": "PARENT"
}
]
Tests
- Add tests in tests/infra/test_thrift_parser.py
- Verified the script with all(1260) commits of common/thrift.
Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
---
M bin/jenkins/critique-gerrit-review.py
A bin/jenkins/thrift_parser.py
A tests/infra/test_thrift_parser.py
3 files changed, 364 insertions(+), 94 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/22264/4
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 10: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/thrift_parser.py
File bin/jenkins/thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/thrift_parser.py@122
PS9, Line 122: turn str(toks)
> I think it won't be hard. Now 'optionalCommaOrSemi' uses 'SEMI' which is Su
Sure. Thanks for checking.
Please file a follow up JIRA after this.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 10
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Jan 2025 15:46:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/critique-gerrit-review.py@270
PS9, Line 270: print(f"Removed struct {struct_name}")
> Should this get a warning? I guess if it was unused elsewhere, it's not an
I think it's ok if it's only used in an optional field and that field is also removed. If it's used in a required field, the following code will report a warning of "Deleting required field..."
It could also be used in a method definition. But currently all the methods of CatalogService are in the same form of single parameter and single response:
service CatalogService {
TDdlExecResponse ExecDdl(1: TDdlExecRequest req);
TGetCatalogObjectResponse GetCatalogObject(1: TGetCatalogObjectRequest req);
TGetPartitionStatsResponse GetPartitionStats(1: TGetPartitionStatsRequest req);
TResetMetadataResponse ResetMetadata(1: TResetMetadataRequest req);
TUpdateCatalogResponse UpdateCatalog(1: TUpdateCatalogRequest req);
TGetFunctionsResponse GetFunctions(1: TGetFunctionsRequest req);
TPrioritizeLoadResponse PrioritizeLoad(1: TPrioritizeLoadRequest req);
TGetPartialCatalogObjectResponse GetPartialCatalogObject(
1: TGetPartialCatalogObjectRequest req);
TUpdateTableUsageResponse UpdateTableUsage(1: TUpdateTableUsageRequest req);
TGetNullPartitionNameResponse GetNullPartitionName(1: TGetNullPartitionNameRequest req);
TGetLatestCompactionsResponse GetLatestCompactions(1: TGetLatestCompactionsRequest req);
}
We usually change the definition of T*Request or T*Response, but not the method definitions. So I think we are still safe. But in the long term, we do need to compare the method definitions as well. As this patch is to resolve the false positive warnings, I hope we can keep it simple and do that in a seperate patch.
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/thrift_parser.py
File bin/jenkins/thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/thrift_parser.py@22
PS9, Line 22: # TODO: Consider using thriftpy2.parser. Need it to expose lineno.
> Why didn't you use it? Too many other dependencies? Also seems like you fig
thriftpy2.parser doesn't provide lineno: https://github.com/Thriftpy/thriftpy2/issues/298
We need to either hack it to expose lineno or try other tools.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 9
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Sat, 04 Jan 2025 00:23:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Riza Suminto, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22264
to look at the new patch set (#5).
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
IMPALA-13305: Better thrift compatibility checks based on pyparsing
There are some false positive warnings reported by
critique-gerrit-review.py when adding a new thrift struct that has
required fields. This patch leverages pyparsing to analyze the
thrift file changes. So we can identify whether the new required field
is added in an existing struct.
thrift_parser.py adds a simple thrift grammar parser to parse a thrift
file into an AST. It basically consists of pyparsing.ParseResults and
some customized classes to inject the line number, i.e.
thrift_parser.ThriftField and thrift_parser.ThriftEnumItem.
Import thrift_parser to parse the current version of a thrift file and
the old version of it before the commit. critique-gerrit-review.py
then compares the structs and enums to report these warnings:
- A required field is deleted in an existing struct.
- A new required field is added in an existing struct.
- An existing field is renamed.
- The qualifier (required/optional) of a field is changed.
- The type of a field is changed.
- An enum item is removed.
- Enum items are reordered.
Only thrift files used in both catalogd and impalad are checked. This is
the same as the current version. We can further improve this by
analyzing all RPCs used between impalad and catalogd to get all thrift
struct/enums used in them.
Warning examples for commit e48af8c04:
"common/thrift/StatestoreService.thrift": [
{
"message": "Renaming field 'sequence' to 'catalogd_version' in TUpdateCatalogdRequest might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 345,
"side": "REVISION"
}
]
Warning examples for commit 595212b4e:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Adding a required field 'type' in TIcebergPartitionField might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 612,
"side": "REVISION"
}
]
Warning examples for commit c57921225:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Renaming field 'partition_id' to 'spec_id' in TIcebergPartitionSpec might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 606,
"side": "REVISION"
}
],
"common/thrift/CatalogService.thrift": [
{
"message": "Changing field 'iceberg_data_files_fb' from required to optional in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 215,
"side": "REVISION"
},
{
"message": "Adding a required field 'operation' in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 209,
"side": "REVISION"
}
],
"common/thrift/Query.thrift": [
{
"message": "Renaming field 'spec_id' to 'iceberg_params' in TFinalizeParams might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 876,
"side": "REVISION"
}
]
Warning example for commit 2b2cf8d96:
"common/thrift/CatalogService.thrift": [
{
"message": "Enum item FUNCTION_NOT_FOUND=3 changed to TABLE_NOT_LOADED=3 in CatalogLookupStatus. This might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 381,
"side": "REVISION"
}
]
Warning example for commit c01efd096:
"common/thrift/JniCatalog.thrift": [
{
"message": "Removing the enum item TAlterTableType.SET_OWNER=15 might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 107,
"side": "PARENT"
}
]
Tests
- Add tests in tests/infra/test_thrift_parser.py
- Verified the script with all(1260) commits of common/thrift.
Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
---
M bin/jenkins/critique-gerrit-review.py
A bin/jenkins/thrift_parser.py
A tests/infra/test_thrift_parser.py
3 files changed, 365 insertions(+), 94 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/22264/5
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11259/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 7
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Dec 2024 10:35:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 9: Code-Review+1
(4 comments)
http://gerrit.cloudera.org:8080/#/c/22264/8/bin/jenkins/thrift_parser.py
File bin/jenkins/thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/8/bin/jenkins/thrift_parser.py@196
PS8, Line 196: structs = defaultdict()
: enums = defaultdict()
: for top_level_item in parse_results:
: if top_level_item.getName() == 'struct':
: # A dict from field id to thrift_parser.ThriftField
: struct_fields = defaultdict()
: for field in to
> Yeah, it looks cleaner.
Done
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/thrift_parser.py
File bin/jenkins/thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/thrift_parser.py@122
PS9, Line 122: optionalCommaOrSemi
Just a thought, can you make this script report if there is a new field or enum item that ends with optional semicolon and let critique-gerrit-review.py WARN about that unnecessary semicolon?
We are pretty inconsistent, especially in TQueryOptions.
http://gerrit.cloudera.org:8080/#/c/22264/8/tests/infra/test_thrift_parser.py
File tests/infra/test_thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/8/tests/infra/test_thrift_parser.py@30
PS8, Line 30: niversal
> Good point!
Done
http://gerrit.cloudera.org:8080/#/c/22264/8/tests/infra/test_thrift_parser.py@34
PS8, Line 34: assert "TErrorCode" in out
> The full output has 4k lines. Uploaded it in the JIRA: https://issues.apach
thrift_parser_output.txt is helpful. Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 9
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Sat, 04 Jan 2025 01:00:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 7:
(1 comment)
Verified in https://jenkins.impala.io/job/gerrit-auto-critic-test/6/
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py@53
PS3, Line 53: PYPARSING_VERSION = "3.1.4"
> Changed to 3.1.4 since somehow the env in Jenkins can't install this versio
In order to run the new test added in tests/infra, I changed the version in infra/python/deps/requirements.txt to 2.4.7 which is the highest version that can be installed in both python2 and python3. pyparsing-2.0.3 doesn't have pyparsing_common so the test will fail.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 7
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Dec 2024 08:58:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 8:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py@53
PS3, Line 53: PYPARSING_VERSION = "3.1.4"
> In order to run the new test added in tests/infra, I changed the version in
Ack
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py@379
PS3, Line 379: for diff_line in diff.splitlines():
: _, _, path = diff_line.split()
: comments[path].append(get_comment_input(UNCOMMITTED_CHANGE))
: if len(comments) > 0:
> We can move extract_thrift_defs(). But for compare_thrift_structs() and com
Ack
http://gerrit.cloudera.org:8080/#/c/22264/8/bin/jenkins/thrift_parser.py
File bin/jenkins/thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/8/bin/jenkins/thrift_parser.py@196
PS8, Line 196: try:
: contents = check_output(["git", "show", f"{revision}:{file_name}"],
: universal_newlines=True)
: except Exception as e:
: # Usually it's due to file doesn't exist in that revision
: print(f"Failed to read {file_name} of revision {revision}: {e}")
: return None, None
I think this part is better to stay in critique-gerrit-review.py, so this script itself does not call git command at all.
Thus, the function signature can be:
def extract_thrift_defs(thrift_file_content):
Consider testing extract_thrift_defs in test_thrift_parser.py as well.
http://gerrit.cloudera.org:8080/#/c/22264/8/tests/infra/test_thrift_parser.py
File tests/infra/test_thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/8/tests/infra/test_thrift_parser.py@30
PS8, Line 30: py3_path
Can this simply be 'impala-python3'?
http://gerrit.cloudera.org:8080/#/c/22264/8/tests/infra/test_thrift_parser.py@34
PS8, Line 34: assert "TStatus" in output
Is the full output too big to show? I have not run the script in my local machine, but want to see the full output.
assert expected_output == output
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 8
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Jan 2025 20:32:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17837/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 9
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Jan 2025 03:03:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17870/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 10
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Jan 2025 15:08:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 7:
> Patch Set 7: Verified-1
>
> Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/11259/
The failure is
https://jenkins.impala.io/job/ubuntu-20.04-from-scratch/4084/testReport/junit/org.apache.impala.planner/PlannerTest/testHbase/
It's unrelated to this patch.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 7
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Dec 2024 23:10:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatability checks based on pyparsing
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatability checks based on pyparsing
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG@7
PS3, Line 7: compatability
nit: compatibility
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Dec 2024 16:33:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13305: Better thrift compatability checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Riza Suminto, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22264
to look at the new patch set (#3).
Change subject: IMPALA-13305: Better thrift compatability checks based on pyparsing
......................................................................
IMPALA-13305: Better thrift compatability checks based on pyparsing
There are some false positive warnings reported by
critique-gerrit-review.py when adding a new thrift struct that has
required fields. This patch leverages pyparsing to analyze the
thrift file changes. So we can identify whether the new required field
is added in an existing struct.
thrift_parser.py adds a simple thrift gramma parser to parse a thrift
file into an AST. It basically consists of pyparsing.ParseResults and
some customized classes to inject the line number, i.e.
thrift_parser.ThriftField and thrift_parser.ThriftEnumItem.
The thrift_parser is used to parse the current version of a thrift file
and the old version of it before the commit. critique-gerrit-review.py
then compares the structs and enums to report these warnings:
- A required field is deleted in an existing struct.
- A new required field is added in an existing struct.
- An existing field is renamed.
- The qualifier (required/optional) of a field is changed.
- The type of a field is changed.
- An enum item is removed.
- Enum items are reordered.
Only thrift files used in both catalogd and impalad are checked. This is
the same as the current version. We can further improve this by
analyzing all RPCs used between impalad and catalogd to get all thrift
struct/enums used in them.
Warning examples for commit e48af8c04:
"common/thrift/StatestoreService.thrift": [
{
"message": "Renaming field 'sequence' to 'catalogd_version' in TUpdateCatalogdRequest might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 345,
"side": "REVISION"
}
]
Warning examples for commit 595212b4e:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Adding a required field 'type' in TIcebergPartitionField might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 612,
"side": "REVISION"
}
]
Warning examples for commit c57921225:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Renaming field 'partition_id' to 'spec_id' in TIcebergPartitionSpec might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 606,
"side": "REVISION"
}
],
"common/thrift/CatalogService.thrift": [
{
"message": "Changing field 'iceberg_data_files_fb' from required to optional in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 215,
"side": "REVISION"
},
{
"message": "Adding a required field 'operation' in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 209,
"side": "REVISION"
}
],
"common/thrift/Query.thrift": [
{
"message": "Renaming field 'spec_id' to 'iceberg_params' in TFinalizeParams might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 876,
"side": "REVISION"
}
]
Warning example for commit 2b2cf8d96:
"common/thrift/CatalogService.thrift": [
{
"message": "Enum item FUNCTION_NOT_FOUND=3 changed to TABLE_NOT_LOADED=3 in CatalogLookupStatus. This might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 381,
"side": "REVISION"
}
]
Warning example for commit c01efd096:
"common/thrift/JniCatalog.thrift": [
{
"message": "Removing the enum item TAlterTableType.SET_OWNER=15 might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 107,
"side": "PARENT"
}
]
Tests
- Verified the script with all(1260) commits of common/thrift.
Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
---
M bin/jenkins/critique-gerrit-review.py
A bin/jenkins/thrift_parser.py
2 files changed, 320 insertions(+), 94 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/22264/3
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 9: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/critique-gerrit-review.py@270
PS9, Line 270: print(f"Removed struct {struct_name}")
> I think it's ok if it's only used in an optional field and that field is al
Ack
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/thrift_parser.py
File bin/jenkins/thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/thrift_parser.py@22
PS9, Line 22: # TODO: Consider using thriftpy2.parser. Need it to expose lineno.
> thriftpy2.parser doesn't provide lineno: https://github.com/Thriftpy/thrift
Ack
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 9
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Sat, 04 Jan 2025 00:28:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13305: Better thrift compatability checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22264
to look at the new patch set (#2).
Change subject: IMPALA-13305: Better thrift compatability checks based on pyparsing
......................................................................
IMPALA-13305: Better thrift compatability checks based on pyparsing
There are some false positive warnings reported by
critique-gerrit-review.py when adding a new thrift struct that has
required fields. This patch leverages pyparsing to analyze the
thrift file changes. So we can identify whether the new required field
is added in an existing struct.
thrift_parser.py adds a simple thrift gramma parser to parse a thrift
file into an AST. It basically consists of pyparsing.ParseResults and
some customized classes to inject the line number, i.e.
thrift_parser.ThriftField and thrift_parser.ThriftEnumItem.
The thrift_parser is used to parse the current version of a thrift file
and the old version of it before the commit. critique-gerrit-review.py
then compares the structs and enums to report these warnings:
- A required field is deleted in an existing struct.
- A new required field is added in an existing struct.
- An existing field is renamed.
- The qualifier (required/optional) of a field is changed.
- The type of a field is changed.
- An enum item is removed.
- Enum items are reordered.
Only thrift files used in both catalogd and impalad are checked. This is
the same as the current version. We can further improve this by
analyzing all RPCs used between impalad and catalogd to get all thrift
struct/enums used in them.
Warning examples for commit e48af8c04:
"common/thrift/StatestoreService.thrift": [
{
"message": "Renaming field 'sequence' to 'catalogd_version' in TUpdateCatalogdRequest might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 345,
"side": "REVISION"
}
]
Warning examples for commit 595212b4e:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Adding a required field 'type' in TIcebergPartitionField might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 612,
"side": "REVISION"
}
]
Warning examples for commit c57921225:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Renaming field 'partition_id' to 'spec_id' in TIcebergPartitionSpec might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 606,
"side": "REVISION"
}
],
"common/thrift/CatalogService.thrift": [
{
"message": "Changing field 'iceberg_data_files_fb' from required to optional in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 215,
"side": "REVISION"
},
{
"message": "Adding a required field 'operation' in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 209,
"side": "REVISION"
}
],
"common/thrift/Query.thrift": [
{
"message": "Renaming field 'spec_id' to 'iceberg_params' in TFinalizeParams might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 876,
"side": "REVISION"
}
]
Warning example for commit 2b2cf8d96:
"common/thrift/CatalogService.thrift": [
{
"message": "Enum item FUNCTION_NOT_FOUND=3 changed to TABLE_NOT_LOADED=3 in CatalogLookupStatus. This might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 381,
"side": "REVISION"
}
]
Warning example for commit c01efd096:
"common/thrift/JniCatalog.thrift": [
{
"message": "Removing the enum item TAlterTableType.SET_OWNER=15 might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 107,
"side": "REVISION"
}
]
Tests
- Verified the script with all(1260) commits of common/thrift.
Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
---
M bin/jenkins/critique-gerrit-review.py
A bin/jenkins/thrift_parser.py
2 files changed, 320 insertions(+), 94 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/22264/2
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 2
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 4:
(10 comments)
Thanks for the quick review!
http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG@7
PS3, Line 7: compatibility
> nit: compatibility
Done
http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG@15
PS3, Line 15: gramma
> nit: grammar
Done
http://gerrit.cloudera.org:8080/#/c/22264/3//COMMIT_MSG@20
PS3, Line 20: Import thrift_parser to pars
> nit: Import thrift_parser to
Done
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py@53
PS3, Line 53: PYPARSING_VERSION = "3.2.0"
> Do we need to reconcile this version with another one defined and used here
Oh, I just realize we already use pyparsing in other place! I think it's ok since this script uses the virtualenv set up at L127. They are independent. I simply choose the latest version of pyparsing.
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py@379
PS3, Line 379:
: # All clean. Run script and comment any changes.
: check_call(['./testdata/bin/restore-stats-on-planner-tests.py'], stdout=DEVNULL)
: diff = check_output(
> What do you think of moving function definitions of these into thrift_parse
We can move extract_thrift_defs(). But for compare_thrift_structs() and compare_thrift_enums(), they use get_comment_input() defined here. So might be better to keep them here.
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py
File bin/jenkins/thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@19
PS3, Line 19: Developed based on
> nit: Developed based on example at:
Done
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@20
PS3, Line 20: https://github.com/pyparsing/pyparsing/blob/c4cf4a5/examples/protobuf_parser.p
> Consider using permalink:
Done
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@54
PS3, Line 54: Word(alphas + "_", bodyChars=alphanums + "_.").setName("identifier")
> Consider using permalink:
Done
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@186
PS3, Line 186: thrift_parser.ignore(pythonStyleComment)
> nit:
Done
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/thrift_parser.py@195
PS3, Line 195: ct a dict of
> Can you write a simple test to check output of this script? Maybe put it un
Done
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Dec 2024 02:50:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 10:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11285/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 10
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Jan 2025 19:33:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
IMPALA-13305: Better thrift compatibility checks based on pyparsing
There are some false positive warnings reported by
critique-gerrit-review.py when adding a new thrift struct that has
required fields. This patch leverages pyparsing to analyze the
thrift file changes. So we can identify whether the new required field
is added in an existing struct.
thrift_parser.py adds a simple thrift grammar parser to parse a thrift
file into an AST. It basically consists of pyparsing.ParseResults and
some customized classes to inject the line number, i.e.
thrift_parser.ThriftField and thrift_parser.ThriftEnumItem.
Import thrift_parser to parse the current version of a thrift file and
the old version of it before the commit. critique-gerrit-review.py
then compares the structs and enums to report these warnings:
- A required field is deleted in an existing struct.
- A new required field is added in an existing struct.
- An existing field is renamed.
- The qualifier (required/optional) of a field is changed.
- The type of a field is changed.
- An enum item is removed.
- Enum items are reordered.
Only thrift files used in both catalogd and impalad are checked. This is
the same as the current version. We can further improve this by
analyzing all RPCs used between impalad and catalogd to get all thrift
struct/enums used in them.
Warning examples for commit e48af8c04:
"common/thrift/StatestoreService.thrift": [
{
"message": "Renaming field 'sequence' to 'catalogd_version' in TUpdateCatalogdRequest might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 345,
"side": "REVISION"
}
]
Warning examples for commit 595212b4e:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Adding a required field 'type' in TIcebergPartitionField might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 612,
"side": "REVISION"
}
]
Warning examples for commit c57921225:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Renaming field 'partition_id' to 'spec_id' in TIcebergPartitionSpec might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 606,
"side": "REVISION"
}
],
"common/thrift/CatalogService.thrift": [
{
"message": "Changing field 'iceberg_data_files_fb' from required to optional in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 215,
"side": "REVISION"
},
{
"message": "Adding a required field 'operation' in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 209,
"side": "REVISION"
}
],
"common/thrift/Query.thrift": [
{
"message": "Renaming field 'spec_id' to 'iceberg_params' in TFinalizeParams might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 876,
"side": "REVISION"
}
]
Warning example for commit 2b2cf8d96:
"common/thrift/CatalogService.thrift": [
{
"message": "Enum item FUNCTION_NOT_FOUND=3 changed to TABLE_NOT_LOADED=3 in CatalogLookupStatus. This might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 381,
"side": "REVISION"
}
]
Warning example for commit c01efd096:
"common/thrift/JniCatalog.thrift": [
{
"message": "Removing the enum item TAlterTableType.SET_OWNER=15 might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 107,
"side": "PARENT"
}
]
Warning example for commit 374783c55:
"common/thrift/Query.thrift": [
{
"message": "Changing type of field 'enabled_runtime_filter_types' from PlanNodes.TEnabledRuntimeFilterTypes to set<PlanNodes.TRuntimeFilterType> in TQueryOptions might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 449,
"side": "REVISION"
}
Tests
- Add tests in tests/infra/test_thrift_parser.py
- Verified the script with all(1260) commits of common/thrift.
Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Reviewed-on: http://gerrit.cloudera.org:8080/22264
Reviewed-by: Riza Suminto <ri...@cloudera.com>
Reviewed-by: Michael Smith <mi...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M bin/jenkins/critique-gerrit-review.py
A bin/jenkins/thrift_parser.py
M infra/python/deps/requirements.txt
A tests/infra/test_thrift_parser.py
4 files changed, 403 insertions(+), 95 deletions(-)
Approvals:
Riza Suminto: Looks good to me, but someone else must approve
Michael Smith: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 11
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 10: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 10
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Jan 2025 02:00:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 10:
(1 comment)
Fixed an issue that complex field types like "list<string>" are just shown as first level types like "list". Updated the test for that.
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/thrift_parser.py
File bin/jenkins/thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/thrift_parser.py@122
PS9, Line 122: turn str(toks)
> Just a thought, can you make this script report if there is a new field or
I think it won't be hard. Now 'optionalCommaOrSemi' uses 'SEMI' which is Suppress(';'). So the semicolon is ignored in the result tokens. We can simply use
optionalCommaOrSemi = Optional("," | ";")
Then check whether the last token is the comma or semicolon in create_thrift_field().
However, currently we just check thrift files related to catalogd RPCs. We'd better add a new method in critique-gerrit-review.py to check all thrift files about this. Might be more clean to do in a separate patch.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 10
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Jan 2025 14:43:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13305: Better thrift compatability checks based on pyparsing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatability checks based on pyparsing
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17787/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 3
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Dec 2024 01:33:33 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Riza Suminto, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22264
to look at the new patch set (#7).
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
IMPALA-13305: Better thrift compatibility checks based on pyparsing
There are some false positive warnings reported by
critique-gerrit-review.py when adding a new thrift struct that has
required fields. This patch leverages pyparsing to analyze the
thrift file changes. So we can identify whether the new required field
is added in an existing struct.
thrift_parser.py adds a simple thrift grammar parser to parse a thrift
file into an AST. It basically consists of pyparsing.ParseResults and
some customized classes to inject the line number, i.e.
thrift_parser.ThriftField and thrift_parser.ThriftEnumItem.
Import thrift_parser to parse the current version of a thrift file and
the old version of it before the commit. critique-gerrit-review.py
then compares the structs and enums to report these warnings:
- A required field is deleted in an existing struct.
- A new required field is added in an existing struct.
- An existing field is renamed.
- The qualifier (required/optional) of a field is changed.
- The type of a field is changed.
- An enum item is removed.
- Enum items are reordered.
Only thrift files used in both catalogd and impalad are checked. This is
the same as the current version. We can further improve this by
analyzing all RPCs used between impalad and catalogd to get all thrift
struct/enums used in them.
Warning examples for commit e48af8c04:
"common/thrift/StatestoreService.thrift": [
{
"message": "Renaming field 'sequence' to 'catalogd_version' in TUpdateCatalogdRequest might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 345,
"side": "REVISION"
}
]
Warning examples for commit 595212b4e:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Adding a required field 'type' in TIcebergPartitionField might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 612,
"side": "REVISION"
}
]
Warning examples for commit c57921225:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Renaming field 'partition_id' to 'spec_id' in TIcebergPartitionSpec might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 606,
"side": "REVISION"
}
],
"common/thrift/CatalogService.thrift": [
{
"message": "Changing field 'iceberg_data_files_fb' from required to optional in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 215,
"side": "REVISION"
},
{
"message": "Adding a required field 'operation' in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 209,
"side": "REVISION"
}
],
"common/thrift/Query.thrift": [
{
"message": "Renaming field 'spec_id' to 'iceberg_params' in TFinalizeParams might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 876,
"side": "REVISION"
}
]
Warning example for commit 2b2cf8d96:
"common/thrift/CatalogService.thrift": [
{
"message": "Enum item FUNCTION_NOT_FOUND=3 changed to TABLE_NOT_LOADED=3 in CatalogLookupStatus. This might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 381,
"side": "REVISION"
}
]
Warning example for commit c01efd096:
"common/thrift/JniCatalog.thrift": [
{
"message": "Removing the enum item TAlterTableType.SET_OWNER=15 might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 107,
"side": "PARENT"
}
]
Tests
- Add tests in tests/infra/test_thrift_parser.py
- Verified the script with all(1260) commits of common/thrift.
Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
---
M bin/jenkins/critique-gerrit-review.py
A bin/jenkins/thrift_parser.py
M infra/python/deps/requirements.txt
A tests/infra/test_thrift_parser.py
4 files changed, 376 insertions(+), 95 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/22264/7
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 7
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/22264/3/bin/jenkins/critique-gerrit-review.py@53
PS3, Line 53:
> Oh, I just realize we already use pyparsing in other place! I think it's ok
Changed to 3.1.4 since somehow the env in Jenkins can't install this version.:
+ /var/lib/jenkins/workspace/gerrit-auto-critic-test/Impala/bin/jenkins/critique-gerrit-review.py --dryrun
Requirement already satisfied: wheel in /usr/lib/python3/dist-packages (0.34.2)
Collecting flake8==7.1.1
Using cached flake8-7.1.1-py2.py3-none-any.whl (57 kB)
Processing /var/lib/jenkins/.cache/pip/wheels/d1/a9/a9/cc03c41c768c6e2f7b2a995d6814c49880b8aacafb70a53d7c/flake8_diff-0.2.2-py3-none-any.whl
ERROR: Could not find a version that satisfies the requirement pyparsing==3.2.0 (from versions: 1.4.6, 1.4.7, 1.4.8, 1.4.11, 1.5.0, 1.5.1, 1.5.2, 1.5.3, 1.5.4, 1.5.5, 1.5.6, 1.5.7, 2.0.0, 2.0.1, 2.0.2, 2.0.3, 2.0.4, 2.0.5, 2.0.6, 2.0.7, 2.1.0, 2.1.1, 2.1.2, 2.1.3, 2.1.4, 2.1.5, 2.1.6, 2.1.7, 2.1.8, 2.1.9, 2.1.10, 2.2.0, 2.2.1, 2.2.2, 2.3.0, 2.3.1, 2.4.0, 2.4.1.1, 2.4.2a1, 2.4.2, 2.4.3, 2.4.4, 2.4.5, 2.4.6, 2.4.7, 3.0.0a1, 3.0.0a2, 3.0.0b1, 3.0.0b2, 3.0.0b3, 3.0.0rc1, 3.0.0rc2, 3.0.0, 3.0.1, 3.0.2, 3.0.3, 3.0.4, 3.0.5, 3.0.6, 3.0.7, 3.0.8, 3.0.9, 3.1.0a1, 3.1.0b1, 3.1.0b2, 3.1.0, 3.1.1, 3.1.2, 3.1.3, 3.1.4)
ERROR: No matching distribution found for pyparsing==3.2.0
Traceback (most recent call last):
File "/var/lib/jenkins/workspace/gerrit-auto-critic-test/Impala/bin/jenkins/critique-gerrit-review.py", line 432, in <module>
setup_virtualenv()
File "/var/lib/jenkins/workspace/gerrit-auto-critic-test/Impala/bin/jenkins/critique-gerrit-review.py", line 129, in setup_virtualenv
check_call([PIP_PATH, "install",
File "/usr/lib/python3.8/subprocess.py", line 364, in check_call
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['gerrit_critic_venv/bin/pip3', 'install', 'wheel', 'flake8==7.1.1', 'flake8-diff==0.2.2', 'pyparsing==3.2.0']' returned non-zero exit status 1.
https://jenkins.impala.io/job/gerrit-auto-critic-test/2/console
http://gerrit.cloudera.org:8080/#/c/22264/6/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/22264/6/bin/jenkins/critique-gerrit-review.py@334
PS6, Line 334: # Importing thrift_parser depends on pyparsing being installed in setup_virtualenv().
: from thrift_parser import extract_thrift_defs
This still failed by "ModuleNotFoundError: No module named 'pyparsing'"
https://jenkins.impala.io/job/gerrit-auto-critic-test/3/console
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 6
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Dec 2024 04:50:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 9:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/22264/8/bin/jenkins/thrift_parser.py
File bin/jenkins/thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/8/bin/jenkins/thrift_parser.py@196
PS8, Line 196: structs = defaultdict()
: enums = defaultdict()
: for top_level_item in parse_results:
: if top_level_item.getName() == 'struct':
: # A dict from field id to thrift_parser.ThriftField
: struct_fields = defaultdict()
: for field in to
> I think this part is better to stay in critique-gerrit-review.py, so this s
Yeah, it looks cleaner.
For testing extract_thrift_defs(), it's hard to invoke the method directly in the test code which runs in Python2. So I modified the main method to use this method.
http://gerrit.cloudera.org:8080/#/c/22264/8/tests/infra/test_thrift_parser.py
File tests/infra/test_thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/8/tests/infra/test_thrift_parser.py@30
PS8, Line 30: niversal
> Can this simply be 'impala-python3'?
Good point!
http://gerrit.cloudera.org:8080/#/c/22264/8/tests/infra/test_thrift_parser.py@34
PS8, Line 34: assert "TErrorCode" in out
> Is the full output too big to show? I have not run the script in my local m
The full output has 4k lines. Uploaded it in the JIRA: https://issues.apache.org/jira/secure/attachment/13073760/thrift_parser_output.txt
I think it'd be better to verify the content of it since whenever there is a thrift change, the full output changes as well. So I choose TStatus and TErrorCode here which are less likely to be changed.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 9
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Jan 2025 02:40:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 5:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/17801/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 5
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Dec 2024 03:23:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has removed a vote on this change.
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Removed Verified-1 by Impala Public Jenkins <im...@cloudera.com>
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 7
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 6:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/17802/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 6
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Dec 2024 03:32:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 7:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/17804/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 7
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Dec 2024 09:20:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 7:
> Patch Set 7:
>
> Build Failed
>
> https://jenkins.impala.io/job/gerrit-code-review-checks/17804/ : Initial code review checks failed. See linked job for details on the failure.
The failure is
Successfully built impala-shell bitarray kerberos prettytable sasl thrift
ERROR: Exception:
Traceback (most recent call last):
File "/home/ubuntu/Impala/shell/build/python2_venv/local/lib/python2.7/site-packages/pip/_internal/cli/base_command.py", line 223, in _main
status = self.run(options, args)
File "/home/ubuntu/Impala/shell/build/python2_venv/local/lib/python2.7/site-packages/pip/_internal/cli/req_command.py", line 180, in wrapper
return func(self, options, args)
File "/home/ubuntu/Impala/shell/build/python2_venv/local/lib/python2.7/site-packages/pip/_internal/commands/install.py", line 404, in run
pycompile=options.compile,
File "/home/ubuntu/Impala/shell/build/python2_venv/local/lib/python2.7/site-packages/pip/_internal/req/__init__.py", line 90, in install_given_reqs
pycompile=pycompile,
File "/home/ubuntu/Impala/shell/build/python2_venv/local/lib/python2.7/site-packages/pip/_internal/req/req_install.py", line 824, in install
requested=self.user_supplied,
File "/home/ubuntu/Impala/shell/build/python2_venv/local/lib/python2.7/site-packages/pip/_internal/operations/install/wheel.py", line 845, in install_wheel
requested=requested,
File "/home/ubuntu/Impala/shell/build/python2_venv/local/lib/python2.7/site-packages/pip/_internal/operations/install/wheel.py", line 674, in _install_wheel
file.save()
File "/home/ubuntu/Impala/shell/build/python2_venv/local/lib/python2.7/site-packages/pip/_internal/operations/install/wheel.py", line 439, in save
with self._zip_file.open(zipinfo) as f:
File "/usr/lib/python2.7/zipfile.py", line 994, in open
raise BadZipfile("Bad magic number for file header")
BadZipfile: Bad magic number for file header
Installing collected packages: bitarray, configparser, kerberos, prettytable, six, sasl, sqlparse, thrift, pure-sasl, thrift-sasl, impala-shell
ERROR in ./make_shell_tarball.sh at line 64: pip install --cache ${PIP_CACHE} \
https://jenkins.impala.io/job/ubuntu-20.04-build-only-ARM/3401/
But the rerun succeeded:
https://jenkins.impala.io/job/ubuntu-20.04-build-only-ARM/3402
Might be unrelated to this patch.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 7
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Dec 2024 10:35:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 10: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 10
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Jan 2025 19:33:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/thrift_parser.py
File bin/jenkins/thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/thrift_parser.py@122
PS9, Line 122: turn str(toks)
> Sure. Thanks for checking.
Sure. Filed IMPALA-13646.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 11
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 17 Jan 2025 01:37:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13305: Better thrift compatability checks based on pyparsing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatability checks based on pyparsing
......................................................................
Patch Set 1:
(29 comments)
gerrit-auto-critic failed. You can reproduce it locally using command:
python3 bin/jenkins/critique-gerrit-review.py --dryrun
To run it, you might need a virtual env with Python3's venv installed.
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/critique-gerrit-review.py@308
PS1, Line 308: +
flake8: W504 line break after binary operator
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/critique-gerrit-review.py@315
PS1, Line 315: +
flake8: W504 line break after binary operator
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/critique-gerrit-review.py@334
PS1, Line 334: +
flake8: W504 line break after binary operator
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/critique-gerrit-review.py@354
PS1, Line 354: +
flake8: W504 line break after binary operator
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/critique-gerrit-review.py@361
PS1, Line 361: +
flake8: W504 line break after binary operator
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py
File bin/jenkins/thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@27
PS1, Line 27: from pyparsing import (
flake8: F401 'pyparsing.Keyword' imported but unused
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@27
PS1, Line 27: from pyparsing import (
flake8: F401 'pyparsing.Dict' imported but unused
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@57
PS1, Line 57:
flake8: W291 trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@57
PS1, Line 57: kwds = """typedef namespace exception struct union required optional enum extends
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@65
PS1, Line 65: L
flake8: F821 undefined name 'LIST_'
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@65
PS1, Line 65: S
flake8: F821 undefined name 'SET_'
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@66
PS1, Line 66: M
flake8: F821 undefined name 'MAP_'
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@71
PS1, Line 71: T
flake8: F821 undefined name 'TRUE_'
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@71
PS1, Line 71: F
flake8: F821 undefined name 'FALSE_'
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@86
PS1, Line 86: def create_thrift_field(src_string, locn, toks):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@94
PS1, Line 94: fieldDefn = (
flake8: E305 expected 2 blank lines after class or function definition, found 1
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@97
PS1, Line 97: R
flake8: F821 undefined name 'REQUIRED_'
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@97
PS1, Line 97: O
flake8: F821 undefined name 'OPTIONAL_'
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@106
PS1, Line 106: S
flake8: F821 undefined name 'STRUCT_'
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@108
PS1, Line 108: U
flake8: F821 undefined name 'UNION_'
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@110
PS1, Line 110: class ThriftEnumItem:
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@119
PS1, Line 119: def create_thrift_enum_item(src_string, locn, toks):
flake8: E302 expected 2 blank lines, found 1
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@124
PS1, Line 124: enumDefn = (
flake8: E305 expected 2 blank lines after class or function definition, found 1
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@125
PS1, Line 125: E
flake8: F821 undefined name 'ENUM_'
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@142
PS1, Line 142: T
flake8: F821 undefined name 'THROWS_'
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@148
PS1, Line 148: S
flake8: F821 undefined name 'SERVICE_'
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@148
PS1, Line 148: E
flake8: F821 undefined name 'EXTENDS_'
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@151
PS1, Line 151: T
flake8: F821 undefined name 'TYPEDEF_'
http://gerrit.cloudera.org:8080/#/c/22264/1/bin/jenkins/thrift_parser.py@183
PS1, Line 183:
flake8: W292 no newline at end of file
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sun, 29 Dec 2024 07:37:08 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13305: Better thrift compatability checks based on pyparsing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatability checks based on pyparsing
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17770/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 1
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sun, 29 Dec 2024 08:01:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Riza Suminto, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22264
to look at the new patch set (#6).
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
IMPALA-13305: Better thrift compatibility checks based on pyparsing
There are some false positive warnings reported by
critique-gerrit-review.py when adding a new thrift struct that has
required fields. This patch leverages pyparsing to analyze the
thrift file changes. So we can identify whether the new required field
is added in an existing struct.
thrift_parser.py adds a simple thrift grammar parser to parse a thrift
file into an AST. It basically consists of pyparsing.ParseResults and
some customized classes to inject the line number, i.e.
thrift_parser.ThriftField and thrift_parser.ThriftEnumItem.
Import thrift_parser to parse the current version of a thrift file and
the old version of it before the commit. critique-gerrit-review.py
then compares the structs and enums to report these warnings:
- A required field is deleted in an existing struct.
- A new required field is added in an existing struct.
- An existing field is renamed.
- The qualifier (required/optional) of a field is changed.
- The type of a field is changed.
- An enum item is removed.
- Enum items are reordered.
Only thrift files used in both catalogd and impalad are checked. This is
the same as the current version. We can further improve this by
analyzing all RPCs used between impalad and catalogd to get all thrift
struct/enums used in them.
Warning examples for commit e48af8c04:
"common/thrift/StatestoreService.thrift": [
{
"message": "Renaming field 'sequence' to 'catalogd_version' in TUpdateCatalogdRequest might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 345,
"side": "REVISION"
}
]
Warning examples for commit 595212b4e:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Adding a required field 'type' in TIcebergPartitionField might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 612,
"side": "REVISION"
}
]
Warning examples for commit c57921225:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Renaming field 'partition_id' to 'spec_id' in TIcebergPartitionSpec might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 606,
"side": "REVISION"
}
],
"common/thrift/CatalogService.thrift": [
{
"message": "Changing field 'iceberg_data_files_fb' from required to optional in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 215,
"side": "REVISION"
},
{
"message": "Adding a required field 'operation' in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 209,
"side": "REVISION"
}
],
"common/thrift/Query.thrift": [
{
"message": "Renaming field 'spec_id' to 'iceberg_params' in TFinalizeParams might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 876,
"side": "REVISION"
}
]
Warning example for commit 2b2cf8d96:
"common/thrift/CatalogService.thrift": [
{
"message": "Enum item FUNCTION_NOT_FOUND=3 changed to TABLE_NOT_LOADED=3 in CatalogLookupStatus. This might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 381,
"side": "REVISION"
}
]
Warning example for commit c01efd096:
"common/thrift/JniCatalog.thrift": [
{
"message": "Removing the enum item TAlterTableType.SET_OWNER=15 might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 107,
"side": "PARENT"
}
]
Tests
- Add tests in tests/infra/test_thrift_parser.py
- Verified the script with all(1260) commits of common/thrift.
Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
---
M bin/jenkins/critique-gerrit-review.py
A bin/jenkins/thrift_parser.py
A tests/infra/test_thrift_parser.py
3 files changed, 365 insertions(+), 94 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/22264/6
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 6
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 4:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/17800/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 4
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Dec 2024 03:15:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Riza Suminto, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22264
to look at the new patch set (#10).
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
IMPALA-13305: Better thrift compatibility checks based on pyparsing
There are some false positive warnings reported by
critique-gerrit-review.py when adding a new thrift struct that has
required fields. This patch leverages pyparsing to analyze the
thrift file changes. So we can identify whether the new required field
is added in an existing struct.
thrift_parser.py adds a simple thrift grammar parser to parse a thrift
file into an AST. It basically consists of pyparsing.ParseResults and
some customized classes to inject the line number, i.e.
thrift_parser.ThriftField and thrift_parser.ThriftEnumItem.
Import thrift_parser to parse the current version of a thrift file and
the old version of it before the commit. critique-gerrit-review.py
then compares the structs and enums to report these warnings:
- A required field is deleted in an existing struct.
- A new required field is added in an existing struct.
- An existing field is renamed.
- The qualifier (required/optional) of a field is changed.
- The type of a field is changed.
- An enum item is removed.
- Enum items are reordered.
Only thrift files used in both catalogd and impalad are checked. This is
the same as the current version. We can further improve this by
analyzing all RPCs used between impalad and catalogd to get all thrift
struct/enums used in them.
Warning examples for commit e48af8c04:
"common/thrift/StatestoreService.thrift": [
{
"message": "Renaming field 'sequence' to 'catalogd_version' in TUpdateCatalogdRequest might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 345,
"side": "REVISION"
}
]
Warning examples for commit 595212b4e:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Adding a required field 'type' in TIcebergPartitionField might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 612,
"side": "REVISION"
}
]
Warning examples for commit c57921225:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Renaming field 'partition_id' to 'spec_id' in TIcebergPartitionSpec might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 606,
"side": "REVISION"
}
],
"common/thrift/CatalogService.thrift": [
{
"message": "Changing field 'iceberg_data_files_fb' from required to optional in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 215,
"side": "REVISION"
},
{
"message": "Adding a required field 'operation' in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 209,
"side": "REVISION"
}
],
"common/thrift/Query.thrift": [
{
"message": "Renaming field 'spec_id' to 'iceberg_params' in TFinalizeParams might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 876,
"side": "REVISION"
}
]
Warning example for commit 2b2cf8d96:
"common/thrift/CatalogService.thrift": [
{
"message": "Enum item FUNCTION_NOT_FOUND=3 changed to TABLE_NOT_LOADED=3 in CatalogLookupStatus. This might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 381,
"side": "REVISION"
}
]
Warning example for commit c01efd096:
"common/thrift/JniCatalog.thrift": [
{
"message": "Removing the enum item TAlterTableType.SET_OWNER=15 might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 107,
"side": "PARENT"
}
]
Warning example for commit 374783c55:
"common/thrift/Query.thrift": [
{
"message": "Changing type of field 'enabled_runtime_filter_types' from PlanNodes.TEnabledRuntimeFilterTypes to set<PlanNodes.TRuntimeFilterType> in TQueryOptions might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 449,
"side": "REVISION"
}
Tests
- Add tests in tests/infra/test_thrift_parser.py
- Verified the script with all(1260) commits of common/thrift.
Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
---
M bin/jenkins/critique-gerrit-review.py
A bin/jenkins/thrift_parser.py
M infra/python/deps/requirements.txt
A tests/infra/test_thrift_parser.py
4 files changed, 403 insertions(+), 95 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/22264/10
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 10
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/critique-gerrit-review.py@270
PS9, Line 270: print(f"Removed struct {struct_name}")
Should this get a warning? I guess if it was unused elsewhere, it's not an issue?
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/thrift_parser.py
File bin/jenkins/thrift_parser.py:
http://gerrit.cloudera.org:8080/#/c/22264/9/bin/jenkins/thrift_parser.py@22
PS9, Line 22: # TODO: Consider using thriftpy2.parser. Need it to expose lineno.
Why didn't you use it? Too many other dependencies? Also seems like you figured out lineno.
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 9
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Jan 2025 21:50:13 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22264 )
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
Patch Set 7: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/11259/
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 7
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 Dec 2024 16:00:12 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13305: Better thrift compatibility checks based on pyparsing
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Hello Riza Suminto, Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22264
to look at the new patch set (#9).
Change subject: IMPALA-13305: Better thrift compatibility checks based on pyparsing
......................................................................
IMPALA-13305: Better thrift compatibility checks based on pyparsing
There are some false positive warnings reported by
critique-gerrit-review.py when adding a new thrift struct that has
required fields. This patch leverages pyparsing to analyze the
thrift file changes. So we can identify whether the new required field
is added in an existing struct.
thrift_parser.py adds a simple thrift grammar parser to parse a thrift
file into an AST. It basically consists of pyparsing.ParseResults and
some customized classes to inject the line number, i.e.
thrift_parser.ThriftField and thrift_parser.ThriftEnumItem.
Import thrift_parser to parse the current version of a thrift file and
the old version of it before the commit. critique-gerrit-review.py
then compares the structs and enums to report these warnings:
- A required field is deleted in an existing struct.
- A new required field is added in an existing struct.
- An existing field is renamed.
- The qualifier (required/optional) of a field is changed.
- The type of a field is changed.
- An enum item is removed.
- Enum items are reordered.
Only thrift files used in both catalogd and impalad are checked. This is
the same as the current version. We can further improve this by
analyzing all RPCs used between impalad and catalogd to get all thrift
struct/enums used in them.
Warning examples for commit e48af8c04:
"common/thrift/StatestoreService.thrift": [
{
"message": "Renaming field 'sequence' to 'catalogd_version' in TUpdateCatalogdRequest might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 345,
"side": "REVISION"
}
]
Warning examples for commit 595212b4e:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Adding a required field 'type' in TIcebergPartitionField might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 612,
"side": "REVISION"
}
]
Warning examples for commit c57921225:
"common/thrift/CatalogObjects.thrift": [
{
"message": "Renaming field 'partition_id' to 'spec_id' in TIcebergPartitionSpec might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 606,
"side": "REVISION"
}
],
"common/thrift/CatalogService.thrift": [
{
"message": "Changing field 'iceberg_data_files_fb' from required to optional in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 215,
"side": "REVISION"
},
{
"message": "Adding a required field 'operation' in TIcebergOperationParam might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 209,
"side": "REVISION"
}
],
"common/thrift/Query.thrift": [
{
"message": "Renaming field 'spec_id' to 'iceberg_params' in TFinalizeParams might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 876,
"side": "REVISION"
}
]
Warning example for commit 2b2cf8d96:
"common/thrift/CatalogService.thrift": [
{
"message": "Enum item FUNCTION_NOT_FOUND=3 changed to TABLE_NOT_LOADED=3 in CatalogLookupStatus. This might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 381,
"side": "REVISION"
}
]
Warning example for commit c01efd096:
"common/thrift/JniCatalog.thrift": [
{
"message": "Removing the enum item TAlterTableType.SET_OWNER=15 might break the compatibility between impalad and catalogd/statestore during upgrade",
"line": 107,
"side": "PARENT"
}
]
Tests
- Add tests in tests/infra/test_thrift_parser.py
- Verified the script with all(1260) commits of common/thrift.
Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
---
M bin/jenkins/critique-gerrit-review.py
A bin/jenkins/thrift_parser.py
M infra/python/deps/requirements.txt
A tests/infra/test_thrift_parser.py
4 files changed, 384 insertions(+), 95 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/64/22264/9
--
To view, visit http://gerrit.cloudera.org:8080/22264
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia1dc4112404d0e7c5df94ee9f59a4fe2084b360d
Gerrit-Change-Number: 22264
Gerrit-PatchSet: 9
Gerrit-Owner: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>