apache / apache/iceberg-python

Expression parser docs and pyparsing notes

Đang mở
#2,860 4 bình luận 4 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Python
Star
1.1k
Fork
581
Merge trung bình
1 ngày 17 giờ
Pull request đã merge (30 ngày)
78

Mô tả

Image
[pyiceberg_diag.html](https://github.com/user-attachments/files/24314065/pyiceberg_diag.html)

### Feature Request / Improvement

I'm happy to see that pyparsing is part of the pyiceberg project. I've attached two files that are railroad diagrams for this parser:
- pyiceberg_diag.html - browseable railroad diagram (SVG), non-terrminal elements are links to subdiagrams
- pyiceberg_parser.png - static PNG version of the same diagram

Overall I think the parser is well-structured, and makes good use of the `infix_notation`, `one_of`, and `DelimitedList` helpers.

My suggestions are largely cosmetic, but may add some parse-time performance improvements.

Recommendations to use `set_name` are either for better diagramming, better exception messages, or both.

## `set_results_name()` vs. `set_name()`

I'd like to point out the distinction between `set_name` and `set_results_name`. `set_name` is most useful to describe the expression itself, in abstract. `set_results_name` should be used to mark expressions with some contextual name, so that their parsed value can be retrieved by that name. I usually think of `set_name` describes what the expression _is_, and `set_results_name` is how the expression _is used_.

For example, these expressions should probably use `set_name`, not `set_results_name`:

```python
boolean = one_of(["true", "false"], caseless=True).set_results_name("boolean")
string = sgl_quoted_string.set_results_name("raw_quoted_string")
decimal = common.real().set_results_name("decimal")
integer = common.signed_integer().set_results_name("integer")
literal = Group(string | decimal | integer | boolean).set_results_name("literal")
literal_set = Group(
DelimitedList(string) | DelimitedList(decimal) | DelimitedList(integer) | DelimitedList(boolean)
).set_results_name("literal_set")
```

You'll also find that exception messages are less cryptic with more use of `set_name`.

```python
Word(nums).parse_string("NAN")
# raises ParseException: Expected W:(0-9), found 'NAN'

Word(nums).set_name("integer").parse_string("NAN")
# raises ParseException: Expected integer, found 'NAN'
```

There are 11 calls to set_results_name.

Many of the `set_results_name` calls are probably better as `set_name` calls. I try to reserve `set_results_name` for expressions that need to be referenced in a parse action, or other post-parsing code. "op" is a good example of using `set_results_name`.

"column" is an excellent example of being an exception to this guideline. It is used in many places, but always as the subject of some larger expression. "column" should be defined using both `set_results_name` and `set_name`.

Lastly, you can keep your set_name and set_results_name calls to a minimum:
- insert a call to pyparsing.autoname_elements(), which will call set_name on all locally defined ParserElements that have not already been given a name (making this call before calling the `infix_notation` function will use the names as part of that functions internal expression building)
- use the `expr("results_name")` short-cut for `expr.set_results_name("results_name")`

## Collapse IS/IS NOT calls

You can reduce the number of terms in your parser by merging IS and IS NOT expressions (fewer terms can translate into faster parsing). Here you define separate `is_null` and `not_null` expressions in building null_check:

```python
is_null = column + IS + NULL
not_null = column + IS + NOT + NULL
null_check = is_null | not_null
```

I propose defining an IS_NOT expression, so that null_check can simplify down (and also reduce 2 parse action functions down to one):

```python
IS_NOT = (IS + NOT).add_parse_action(lambda: "IS_NOT")
null_check = column + (IS_NOT | IS)("op") + NULL

@null_check.set_parse_action
def _(result: ParseResults) -> BooleanExpression:
expr_class = IsNull if result.op == "IS" else NotNull
return expr_class(result.column)
```

Similar treatment can be given to IN/NOT IN and LIKE/NOT LIKE.

## Creating the diagram

I wrote this little script to build the attached diagrams:

```python
import pyiceberg.expressions.parser as ibp

ibp.boolean_expression.create_diagram("pyiceberg_parser_diag.html")
```

Run this before making any changes, and you'll get a diagram that is difficult to navigate, probably even difficult to view! Make a few `set_name` calls (like adding `set_name("column")` to `column`) and regenerate the diagram and see how the structure becomes clearer. The non-terminals in the diagram are actually links to their corresponding sub-diagrams, so navigation in a large diagram is much easier. I've gone back and refactored a number of the pyparsing examples, after generating their diagrams.

## Other notes

Not really a pyparsing note, but just offering this alternative style for your if-elif-else chains:

```python
@left_ref.set_parse_action
def _(result: ParseResults) -> BooleanExpression:
op_classes = {
">": GreaterThan,
">=": GreaterThanOrEqual,
"<": LessThan,
"<=": LessThanOrEqual,
"=": EqualTo,
"==": EqualTo,
"!=": NotEqualTo,
"<>": NotEqualTo,
}
op_class = op_classes.get(result.op)
if op_class is not None:
return op_class(result.column, result.literal)
raise ValueError(f"Unsupported operation type: {result.op!r}")
```

I encourage people at work, when echoing erroneous values in an exception, to add the "!r" format marker. This encloses the value in single quotes _and_ expands any non-printing characters to hex notation. A big help for when the parser accidentally includes trailing space in the operator string, and so it doesn't match any of the expected patterns.

## Finally

Again, your parser is fine as-is, these suggestions just may make it a bit easier to maintain, and even a bit faster.

Hướng dẫn đóng góp

Chưa lập chỉ mục được hướng dẫn đóng góp cho kho mã nguồn này

Hướng nghiên cứu

Bắt đầu với các định nghĩa parser trong pyiceberg.expressions.parser và xem xét các lệnh gọi set_results_name hiện có, null_check, cùng các biểu thức IN/NOT IN và LIKE/NOT LIKE. Tạo sơ đồ parser trước và sau các thay đổi như được mô tả trong issue, đồng thời xác minh rằng việc đặt tên cải thiện sơ đồ và các thông báo exception, trong khi các đơn giản hóa operator được đề xuất vẫn giữ nguyên behavior.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
python
Lĩnh vực
backend
Loại issue
Tái cấu trúc
Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức độ hoạt động
Ít trao đổi
Độ rõ ràng
Khá rõ ràng
Mức phù hợp với người mới
52/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.