You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by mi...@apache.org on 2024/08/16 00:16:09 UTC
(impala) 01/02: IMPALA-13240: Add gerrit comments for Thrift/FlatBuffers changes
This is an automated email from the ASF dual-hosted git repository.
michaelsmith pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit db0f0dadf19e8a18bdf8e7c1788e1fe2af9cb675
Author: stiga-huang <hu...@gmail.com>
AuthorDate: Tue Aug 6 12:42:19 2024 +0800
IMPALA-13240: Add gerrit comments for Thrift/FlatBuffers changes
Adds gerrit comments for changes in Thrift/FlatBuffers files that could
break the communication between impalad and catalogd/statestore during
upgrade.
Basically, only new optional fields can be added in Thrift protocol. For
Flatbuffers schemas, we should only add new fields at the end of a table
definition.
Adds a new option (--revision) for critique-gerrit-review.py to specify
the revision (HEAD or a commit, branch, etc). Also adds an option
(--base-revision) to specify the base revision for comparison.
To test the script locally, prepare a virtual env with the virtualenv
package installed:
virtualenv venv
source venv/bin/activate
pip install virtualenv
Then run the script with --dryrun:
python bin/jenkins/critique-gerrit-review.py --dryrun --revision effc9df93
Limitations
- False positive in cases that add new Thrift structs with required
fields and only use those new structs in new optional fields, e.g.
effc9df93 and 72732da9d.
- Might have false positive results on reformat changes due to simple
string checks, e.g. 91d8a8f62.
- Can't check incompatible changes in FlatBuffers files. Just add
general file level comments.
We can integrate DUPCheck in the future to parse the Thrift/FlatBuffers
files to AST and compare the AST instead.
https://github.com/jwjwyoung/DUPChecker
Tests:
- Verified incompatible commits like 012996a06 and 65094a74f.
- Verified posting Gerrit comments from local env using my username.
Change-Id: Ib35fafa50bfd38631312d22464df14d426f55346
Reviewed-on: http://gerrit.cloudera.org:8080/21646
Reviewed-by: Riza Suminto <ri...@cloudera.com>
Tested-by: Quanlong Huang <hu...@gmail.com>
---
bin/jenkins/critique-gerrit-review.py | 235 ++++++++++++++++++++++++++++++----
common/thrift/Data.thrift | 2 +
2 files changed, 211 insertions(+), 26 deletions(-)
diff --git a/bin/jenkins/critique-gerrit-review.py b/bin/jenkins/critique-gerrit-review.py
index c165017c4..d3ce54ec3 100755
--- a/bin/jenkins/critique-gerrit-review.py
+++ b/bin/jenkins/critique-gerrit-review.py
@@ -45,7 +45,6 @@ from os import environ
import os.path
import re
from subprocess import check_call, check_output, Popen, PIPE
-import sys
import virtualenv
FLAKE8_VERSION = "3.9.2"
@@ -60,7 +59,7 @@ FLAKE8_DIFF_PATH = os.path.join(VENV_BIN, "flake8-diff")
LINE_LIMIT = 90
# Source file extensions that we should apply our line limit and whitespace rules to.
-SOURCE_EXTENSIONS = set([".cc", ".h", ".java", ".py", ".sh", ".thrift"])
+SOURCE_EXTENSIONS = {".cc", ".h", ".java", ".py", ".sh", ".thrift"}
# Source file patterns that we exclude from our checks.
EXCLUDE_FILE_PATTERNS = [
@@ -74,6 +73,49 @@ EXCLUDE_FILE_PATTERNS = [
re.compile(r".*/.*\.xml\.py") # Long lines in config template files.
]
+# Thrift files that are not used in communication between impalad and catalogd/statestore
+EXCLUDE_THRIFT_FILES = {
+ "BackendGflags.thrift", # Only used between FE and BE
+ "beeswax.thrift", # Only used between client and impalad
+ "DataSinks.thrift", # Only used in impalads
+ "Descriptors.thrift", # Only used in impalads
+ "ExecStats.thrift", # Only used in impalads
+ "LineageGraph.thrift", # Only used in impalads
+ "NetworkTest.thrift", # Unused in production
+ "Planner.thrift", # Only used in impalads
+ "PlanNodes.thrift", # Only used in impalads
+ "parquet.thrift", # Only used in impalads
+ "ResourceProfile.thrift", # Only used in impalads
+ "SystemTables.thrift", # Only used in impalads
+ "Zip.thrift", # Unused
+}
+
+THRIFT_FILE_COMMENT = (
+ "This file is used in communication between impalad and catalogd/statestore. "
+ "Please make sure impalads can still work with new/old versions of catalogd and "
+ "statestore. Basically only new optional fields can be added.")
+FBS_FILE_COMMENT = (
+ "This file is used in communication between impalad and catalogd/statestore. "
+ "Please make sure impalads can still work with new/old versions of catalogd and "
+ "statestore. Basically only new fields can be added and should be added at the end "
+ "of a table definition.\n"
+ "https://flatbuffers.dev/flatbuffers_guide_writing_schema.html")
+COMMENT_REVISION_SIDE = "REVISION"
+COMMENT_PARENT_SIDE = "PARENT"
+
+# Matches range information like:
+# @@ -229,0 +230,2 @@ struct TColumnStats {
+RANGE_RE = re.compile(r"^@@ -([0-9]*).* \+([0-9]*).*@@\s*(.*)$")
+# Matches required/optional fields like:
+# 7: required i64 num_trues
+REQUIRED_FIELD_RE = re.compile(r"[0-9]*: required \w* \w*")
+OPTIONAL_FIELD_RE = re.compile(r"[0-9]*: optional \w* \w*")
+# Matches include statements like:
+# include "Exprs.thrift"
+INCLUDE_FILE_RE = re.compile(r"include \"(\w+\.thrift)\"")
+THRIFT_WARNING_SUFFIX = (" might break the compatibility between impalad and "
+ "catalogd/statestore during upgrade")
+
def setup_virtualenv():
"""Set up virtualenv with flake8-diff."""
@@ -83,7 +125,15 @@ def setup_virtualenv():
"flake8-diff=={0}".format(FLAKE8_DIFF_VERSION)])
-def get_flake8_comments(revision):
+def get_comment_input(message, line_number=0, side=COMMENT_REVISION_SIDE,
+ context_line="", dryrun=False):
+ comment = {"message": message, "line": line_number, "side": side}
+ if dryrun:
+ comment["context_line"] = context_line
+ return comment
+
+
+def get_flake8_comments(base_revision, revision):
"""Get flake8 warnings for code changes made in the git commit 'revision'.
Returns a dict with file path as keys and a list of CommentInput objects. See
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#review-input
@@ -93,7 +143,6 @@ def get_flake8_comments(revision):
flake8_env = os.environ.copy()
flake8_env["PATH"] = "{0}:{1}".format(VENV_BIN, flake8_env["PATH"])
- base_revision = "{0}^".format(revision)
flake8_diff_proc = Popen(
[FLAKE8_DIFF_PATH, "--standard-flake8-output", "--color", "off", base_revision,
revision],
@@ -134,7 +183,7 @@ def get_flake8_comments(revision):
return comments
-def get_misc_comments(revision):
+def get_misc_comments(base_revision, revision, dryrun=False):
"""Get miscellaneous warnings for code changes made in the git commit 'revision', e.g.
long lines and trailing whitespace. These warnings are produced by directly parsing the
diff output."""
@@ -143,7 +192,7 @@ def get_misc_comments(revision):
# @@ -128 +133,2 @@ if __name__ == "__main__":
RANGE_RE = re.compile(r"^@@ -[0-9,]* \+([0-9]*).*$")
- diff = check_output(["git", "diff", "-U0", "{0}^..{0}".format(revision)],
+ diff = check_output(["git", "diff", "-U0", "{0}..{1}".format(base_revision, revision)],
universal_newlines=True)
curr_file = None
check_source_file = False
@@ -168,35 +217,154 @@ def get_misc_comments(revision):
curr_line_num = int(match.group(1))
elif diff_line.startswith("+") and check_source_file:
# An added or modified line - check it to see if we should generate warnings.
- add_misc_comments_for_line(comments, diff_line[1:], curr_file, curr_line_num)
+ add_misc_comments_for_line(comments, diff_line[1:], curr_file, curr_line_num,
+ dryrun)
curr_line_num += 1
return comments
-def add_misc_comments_for_line(comments, line, curr_file, curr_line_num):
+def add_misc_comments_for_line(comments, line, curr_file, curr_line_num, dryrun=False):
"""Helper for get_misc_comments to generate comments for 'line' at 'curr_line_num' in
'curr_file' and append them to 'comments'."""
# Check for trailing whitespace.
if line.rstrip() != line:
- comments[curr_file].append(
- {"message": "line has trailing whitespace", "line": curr_line_num})
+ comments[curr_file].append(get_comment_input(
+ "line has trailing whitespace", curr_line_num, context_line=line, dryrun=dryrun))
# Check for long lines. Skip .py files since flake8 already flags long lines.
if len(line) > LINE_LIMIT and os.path.splitext(curr_file)[1] != ".py":
msg = "line too long ({0} > {1})".format(len(line), LINE_LIMIT)
- comments[curr_file].append(
- {"message": msg, "line": curr_line_num})
+ comments[curr_file].append(get_comment_input(
+ msg, curr_line_num, context_line=line, dryrun=dryrun))
if '\t' in line:
- comments[curr_file].append(
- {"message": "tab used for whitespace", "line": curr_line_num})
+ comments[curr_file].append(get_comment_input(
+ "tab used for whitespace", curr_line_num, context_line=line, dryrun=dryrun))
+
+ if 'ThriftDebugString' in line and curr_file.startswith("be/src/"):
+ comments[curr_file].append(get_comment_input(
+ "Please make sure you don't output sensitive data with ThriftDebugString(). "
+ "If so, use impala::RedactedDebugString() instead.",
+ curr_line_num, context_line=line, dryrun=dryrun))
+
+
+def get_catalog_compatibility_comments(base_revision, revision, dryrun=False):
+ """Get comments on Thrift/FlatBuffers changes that might break the communication
+ between impalad and catalogd/statestore"""
+ comments = defaultdict(lambda: [])
+
+ diff = check_output(
+ ["git", "diff", "-U0", "{0}..{1}".format(base_revision, revision),
+ "--", "common/thrift"],
+ universal_newlines=True)
+ curr_file = None
+ check_source_file = False
+ has_concerns = False
+ in_enum_clause = False
+ is_thrift_file = False
+ # Line numbers in the old file and in the new file
+ old_line_num = 0
+ new_line_num = 0
+ for diff_line in diff.splitlines():
+ if diff_line.startswith("--- "):
+ continue
+ elif diff_line.startswith("+++ "):
+ # Start of diff for a file. Add a comment for the previous file if has concerns.
+ if curr_file and check_source_file and has_concerns:
+ comments[curr_file].append(get_comment_input(THRIFT_FILE_COMMENT))
+ # Strip off "+++ b/" to get the file path.
+ curr_file = diff_line[6:]
+ check_source_file = False
+ has_concerns = False
+ in_enum_clause = False
+ is_thrift_file = os.path.splitext(curr_file)[1] == ".thrift"
+ if is_thrift_file and os.path.basename(curr_file) not in EXCLUDE_THRIFT_FILES:
+ check_source_file = True
+ elif check_source_file and diff_line.startswith("@@ "):
+ # Figure out the starting line of the hunk. Examples:
+ # @@ -932,0 +933,5 @@ enum TImpalaQueryOptions {
+ # @@ -55,0 +56 @@ enum TPlanNodeType {
+ # @@ -109 +109 @@ struct THdfsTableSink {
+ # We want to extract the start line for the added lines
+ match = RANGE_RE.match(diff_line)
+ if not match:
+ raise Exception("Pattern did not match diff line:\n{0}".format(diff_line))
+ old_line_num = int(match.group(1))
+ new_line_num = int(match.group(2))
+ in_enum_clause = match.group(3).startswith("enum ")
+ elif check_source_file and diff_line.startswith("-"):
+ # Check if deleting/modifying a required field
+ change = diff_line[1:].strip()
+ if in_enum_clause and not change.startswith("//"):
+ has_concerns = True
+ comments[curr_file].append(get_comment_input(
+ "Modifying/deleting this enum item" + THRIFT_WARNING_SUFFIX,
+ old_line_num, COMMENT_PARENT_SIDE, diff_line, dryrun))
+ elif REQUIRED_FIELD_RE.match(change):
+ has_concerns = True
+ comments[curr_file].append(get_comment_input(
+ "Modifying/deleting this required field" + THRIFT_WARNING_SUFFIX,
+ old_line_num, COMMENT_PARENT_SIDE, diff_line, dryrun))
+ elif OPTIONAL_FIELD_RE.match(change):
+ # Removing an optional field should be OK unless the field number is reused by
+ # a new optional field. Add a general comment on the file.
+ has_concerns = True
+ old_line_num += 1
+ elif is_thrift_file and diff_line.startswith("+"):
+ change = diff_line[1:].strip()
+ # Check includes in Thrift files
+ match = INCLUDE_FILE_RE.match(change)
+ # Case 1: a target file includes a whitelist file, E.g. Frontend.thrift includes
+ # LineageGraph.thrift. Future changes in LineageGraph.thrift might impact
+ # Frontend.thrift so
+ # - LineageGraph.thrift should be removed from the whitelist (i.e.
+ # EXCLUDE_THRIFT_FILES) if it will be used in impalad and catalogd.
+ # - Or developers should make sure the included new fields are read/write only
+ # in impalads or only in catalogd.
+ if match and check_source_file and match.group(1) in EXCLUDE_THRIFT_FILES:
+ comments[curr_file].append(get_comment_input(
+ "Future changes in {0} might break the compatibility between impalad and "
+ "catalogd/statestore. Please remove {0} from EXCLUDE_THRIFT_FILES in "
+ "bin/jenkins/critique-gerrit-review.py or make sure the new fields are "
+ "read/write only in impalads or only in catalogd".format(match.group(1)),
+ new_line_num, context_line=diff_line, dryrun=dryrun))
+ # Case 2: A whitelist file includes a target file, e.g. PlanNodes.thrift includes
+ # Data.thrift. Note that PlanNodes.thrift is supposed to be used inside impalad.
+ # Data.thrift is used in both impalad and catalogd. We should ensure new fields
+ # included from Data.thrift are not set from catalogd.
+ elif (match and not check_source_file
+ and match.group(1) not in EXCLUDE_THRIFT_FILES):
+ comments[curr_file].append(get_comment_input(
+ "Thrift objects in the current file are supposed to be used inside "
+ "impalads. Please make sure new fields includes from {} are not set by "
+ "catalogd.".format(match.group(1)),
+ new_line_num, context_line=diff_line, dryrun=dryrun))
+ elif check_source_file:
+ if REQUIRED_FIELD_RE.match(change):
+ has_concerns = True
+ comments[curr_file].append(get_comment_input(
+ "Modifying/adding this required field" + THRIFT_WARNING_SUFFIX,
+ new_line_num, context_line=diff_line, dryrun=dryrun))
+ new_line_num += 1
+ if curr_file and check_source_file and has_concerns:
+ comments[curr_file].append(get_comment_input(THRIFT_FILE_COMMENT))
+ merge_comments(
+ comments, get_flatbuffers_compatibility_comments(base_revision, revision))
+ return comments
+
+
+def get_flatbuffers_compatibility_comments(base_revision, revision):
+ comments = defaultdict(lambda: [])
+ diff = check_output(
+ ["git", "diff", "--numstat", "{0}..{1}".format(base_revision, revision),
+ "--", "common/fbs"],
+ universal_newlines=True)
+ for diff_line in diff.splitlines():
+ _, _, path = diff_line.split()
+ if os.path.splitext(path)[1] == ".fbs":
+ comments[path].append(get_comment_input(FBS_FILE_COMMENT))
+ return comments
- if 'ThriftDebugString' in line:
- comments[curr_file].append(
- {"message": ("Please make sure you don't output sensitive data with "
- "ThriftDebugString(). If so, use impala::RedactedDebugString() "
- "instead."),
- "line": curr_line_num })
def post_review_to_gerrit(review_input):
"""Post a review to the gerrit patchset. 'review_input' is a ReviewInput JSON object
@@ -221,16 +389,31 @@ def merge_comments(a, b):
if __name__ == "__main__":
parser = ArgumentParser(description="Generate and post gerrit comments")
parser.add_argument("--dryrun", action='store_true',
- help="Don't post comments back to gerrit")
+ help="Don't post comments back to gerrit. Also shows the context "
+ "lines if possible.")
+ parser.add_argument("--revision", default="HEAD",
+ help="The revision to check. Defaults to HEAD. Note that "
+ "flake8-diff only actually works correctly on HEAD. So "
+ "specifying other commits might miss the results of "
+ "flake8-diff.")
+ parser.add_argument("--base-revision",
+ help="The base revision to check. Defaults to the parent commit of"
+ " revision")
args = parser.parse_args()
setup_virtualenv()
- # flake8-diff only actually works correctly on HEAD, so this is the only revision
- # we can correctly handle.
- revision = 'HEAD'
- comments = get_flake8_comments(revision)
- merge_comments(comments, get_misc_comments(revision))
+ revision = args.revision
+ base_revision = args.base_revision if args.base_revision else "{0}^".format(revision)
+ comments = get_flake8_comments(base_revision, revision)
+ merge_comments(comments, get_misc_comments(base_revision, revision, args.dryrun))
+ merge_comments(
+ comments, get_catalog_compatibility_comments(base_revision, revision, args.dryrun))
review_input = {"comments": comments}
+ if len(comments) > 0:
+ review_input["message"] = (
+ "gerrit-auto-critic failed. You can reproduce it locally using command:\n\n"
+ " python2 bin/jenkins/critique-gerrit-review.py --dryrun\n\n"
+ "To run it, you might need a virtual env with virtualenv installed.")
print(json.dumps(review_input, indent=True))
if not args.dryrun:
post_review_to_gerrit(review_input)
diff --git a/common/thrift/Data.thrift b/common/thrift/Data.thrift
index f4a5450a1..b68657f3e 100644
--- a/common/thrift/Data.thrift
+++ b/common/thrift/Data.thrift
@@ -33,6 +33,8 @@ struct TColumnValue {
11: optional i32 date_val
}
+// A result row returned to the client. Used in both impalad and catalogd.
+// Note that catalogd sets the summary of DDL/DMLs.
struct TResultRow {
1: list<TColumnValue> colVals
}