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 <riza.suminto@cloudera.com>
Reviewed-by: Michael Smith <michael.smith@cloudera.com>
Tested-by: Impala Public Jenkins <impala-public-jenkins@cloudera.com>
This commit is contained in:
stiga-huang
2024-12-27 16:41:11 +08:00
committed by Impala Public Jenkins
parent ce6be49c08
commit 777ae104bb
4 changed files with 403 additions and 95 deletions

View File

@@ -39,7 +39,7 @@ pg8000 == 1.10.2
prettytable == 0.7.2
prometheus-client == 0.12.0
psutil == 5.6.3
pyparsing == 2.0.3
pyparsing == 2.4.7
pytest == 2.9.2
py == 1.4.32
pytest-forked == 0.2