[
https://issues.apache.org/jira/browse/IMPALA-13305?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17910471#comment-17910471
]
ASF subversion and git services commented on IMPALA-13305:
----------------------------------------------------------
Commit 777ae104bb1f5e5e0f8d19bd26886fef8084453c in impala's branch
refs/heads/master from stiga-huang
[ https://gitbox.apache.org/repos/asf?p=impala.git;h=777ae104b ]
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 <[email protected]>
Reviewed-by: Michael Smith <[email protected]>
Tested-by: Impala Public Jenkins <[email protected]>
> Better Thrift/FlatBuffers compatibility checks based on AST parsing
> -------------------------------------------------------------------
>
> Key: IMPALA-13305
> URL: https://issues.apache.org/jira/browse/IMPALA-13305
> Project: IMPALA
> Issue Type: Task
> Components: Infrastructure
> Reporter: Quanlong Huang
> Assignee: Quanlong Huang
> Priority: Major
> Attachments: thrift_parser_output.txt
>
>
> This is a follow-up task of IMPALA-13240 where we add basic checks in
> potential incompatible changes in Thrift/FlatBuffers. To have accurate
> checks, we'd better integrate DUPCheker [1] or pyparsing to parse the
> definition codes and compare the AST.
> [1] https://github.com/jwjwyoung/DUPChecker
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
