kaxil commented on code in PR #73926:
URL: https://github.com/apache/airflow/pull/73926#discussion_r4146101910
##########
providers/common/ai/tests/unit/common/ai/operators/test_document_loader.py:
##########
@@ -422,6 +432,54 @@ def
test_docx_missing_raises_optional_feature_exception(self, tmp_path):
with pytest.raises(AirflowOptionalProviderFeatureException):
op.execute(context=MagicMock())
+ @pytest.mark.parametrize(
+ ("blocks", "expected"),
+ [
+ pytest.param(
+ ("Intro", " ", [["Name", "Qty"], ["Apple", "3"]], "Outro"),
+ "Intro\n\n| Name | Qty |\n| Apple | 3 |\n\nOutro",
+ id="tables-in-document-order",
+ ),
+ pytest.param(([["a", "", "c"]],), "| a | | c |",
id="empty-cell-keeps-its-column"),
+ pytest.param(
+ ([["h1", "h2"], ["", ""], ["x", "y"]],),
+ "| h1 | h2 |\n| x | y |",
+ id="empty-row-skipped",
+ ),
+ pytest.param(
+ ("Before", [["", ""]], "After", [["Name", "Qty"]]),
+ "Before\n\nAfter\n\n| Name | Qty |",
+ id="empty-table-skipped",
+ ),
+ pytest.param(
+ ([["line1\nline2", "b"]],),
+ "| line1 line2 | b |",
+ id="cell-line-break-stays-on-row",
+ ),
+ ],
+ )
+ def test_docx_tables(self, blocks, expected):
Review Comment:
No fixture has two adjacent unmerged cells with the same text, so these
cases would also pass if the `cell is previous_cell` check became a
text-equality check. With that swap, `[["0", "0"], ["x", "x"]]` renders as `| 0
|` / `| x |`. Adding a case like `pytest.param(([["0", "0"], ["x", "x"]],), "|
0 | 0 |\n| x | x |", id="equal-adjacent-unmerged-cells-kept")` would lock in
the identity check.
##########
providers/common/ai/src/airflow/providers/common/ai/operators/document_loader.py:
##########
@@ -452,19 +454,56 @@ def _parse_pdf_stream(self, stream: BinaryIO) ->
list[dict[str, Any]]:
def _parse_docx_stream(self, stream: BinaryIO) -> list[dict[str, Any]]:
"""
- Parse a DOCX stream into documents.
+ Parse a DOCX stream into a single document.
- Extracts paragraph text only. Tables, headers, footers, and footnotes
- are not included. For richer DOCX parsing, plug in a dedicated
- extraction tool (``Unstructured``, ``docling``) as a custom parser
- backend.
+ Paragraphs and tables in the document body are extracted in document
+ order. Each table row becomes one "| cell | cell |" line, and a nested
+ table is flattened into its cell. Headers, footers, footnotes, and
+ content controls are not included.
"""
try:
from docx import Document
+ from docx.table import Table
except ImportError as e:
raise AirflowOptionalProviderFeatureException(e)
doc = Document(stream)
- paragraphs = [p.text for p in doc.paragraphs if p.text.strip()]
- text = "\n\n".join(paragraphs)
+ blocks = []
+ for block in doc.iter_inner_content():
+ if isinstance(block, Table):
+ text = "\n".join(f"| {' | '.join(cells)} |" for cells in
self._get_docx_table_rows(block))
Review Comment:
Before this PR, a table could never fail a DOCX parse. Now some tables raise
out of python-docx's `row.cells`. python-docx resolves each vertically merged
cell by recursing up to the merge's first row. So a vertical merge spanning
about 1000 rows hits `RecursionError` on 1.2.0 (1100 rows failed after ~20 s;
300 rows already took 1.1 s), and raising the floor doesn't change that. A
`vMerge="continue"` cell in the first row raises `ValueError: no tr above
topmost tr`. Either one fails the whole file, and in directory mode the whole
batch, where it used to load its paragraphs. Would it be worth catching per
table here and logging a warning, so one odd table is skipped instead of
failing the task?
##########
providers/common/ai/src/airflow/providers/common/ai/operators/document_loader.py:
##########
@@ -452,19 +454,56 @@ def _parse_pdf_stream(self, stream: BinaryIO) ->
list[dict[str, Any]]:
def _parse_docx_stream(self, stream: BinaryIO) -> list[dict[str, Any]]:
"""
- Parse a DOCX stream into documents.
+ Parse a DOCX stream into a single document.
- Extracts paragraph text only. Tables, headers, footers, and footnotes
- are not included. For richer DOCX parsing, plug in a dedicated
- extraction tool (``Unstructured``, ``docling``) as a custom parser
- backend.
+ Paragraphs and tables in the document body are extracted in document
+ order. Each table row becomes one "| cell | cell |" line, and a nested
+ table is flattened into its cell. Headers, footers, footnotes, and
+ content controls are not included.
"""
try:
from docx import Document
+ from docx.table import Table
except ImportError as e:
raise AirflowOptionalProviderFeatureException(e)
doc = Document(stream)
- paragraphs = [p.text for p in doc.paragraphs if p.text.strip()]
- text = "\n\n".join(paragraphs)
+ blocks = []
+ for block in doc.iter_inner_content():
+ if isinstance(block, Table):
+ text = "\n".join(f"| {' | '.join(cells)} |" for cells in
self._get_docx_table_rows(block))
+ else:
+ text = block.text
+ if text.strip():
+ blocks.append(text)
+ text = "\n\n".join(blocks)
return [{"text": text, "metadata": {}}]
+
+ def _get_docx_table_rows(self, table: Table) -> list[list[str]]:
+ rows = []
+ for row in table.rows:
+ cells: list[str] = []
+ previous_cell = None
+ for cell in row.cells:
Review Comment:
Even on 1.1.2+/1.2.0, `row.cells` only returns the cells present in the row,
so a row that starts with `w:gridBefore` shifts its values left under the wrong
headers. Under `| Product | Revenue | Cost |`, a row that omits Product renders
as `| 100 | 80 |`. Padding with `[""] * row.grid_cols_before` before the loop
and `[""] * row.grid_cols_after` after it gives `| | 100 | 80 |` (both
properties exist from 1.1.2). Could you add a test with a real
gridBefore/gridAfter row? None of the current fixtures have one.
##########
providers/common/ai/pyproject.toml:
##########
@@ -147,7 +147,7 @@ dependencies = [
"llama-index-llms-openai>=0.6.8",
]
"pdf" = ["pypdf>=4.0.0"]
-"docx" = ["python-docx>=1.0.0"]
+"docx" = ["python-docx>=1.1.0"]
Review Comment:
I think the floor needs to be `>=1.1.2` rather than `>=1.1.0`. In 1.1.0,
`_Row.cells` is `table.row_cells(self._index)`, which rebuilds the whole
table's cell list on every call and then slices it by column count. That causes
two problems for the new loop. It's quadratic: a 500x5 table took 5.6 s to
parse on 1.1.0 against 0.05 s on 1.2.0, and a 2000-row table took about 90 s.
It also ignores `w:gridBefore`/`w:gridAfter`, so a row with omitted cells pulls
cells from the next row. A header, then `[A, B]` with gridAfter=1, then `[C, D,
E]` comes out as `| A | B | C |` / `| D | E |`. 1.1.1 switched to a per-row
walk, but it pins `lxml<=4.9.2`, which doesn't build on 3.12, so 1.1.2 is the
lowest version that works. The lowest-direct-deps CI job resolves 1.1.0, and
the small regular tables in the tests pass there. The same bump is needed in
the dev group, docs/index.rst and uv.lock, and README.rst:106 still says
`>=1.0.0`.
##########
providers/common/ai/src/airflow/providers/common/ai/operators/document_loader.py:
##########
@@ -452,19 +454,56 @@ def _parse_pdf_stream(self, stream: BinaryIO) ->
list[dict[str, Any]]:
def _parse_docx_stream(self, stream: BinaryIO) -> list[dict[str, Any]]:
"""
- Parse a DOCX stream into documents.
+ Parse a DOCX stream into a single document.
- Extracts paragraph text only. Tables, headers, footers, and footnotes
- are not included. For richer DOCX parsing, plug in a dedicated
- extraction tool (``Unstructured``, ``docling``) as a custom parser
- backend.
+ Paragraphs and tables in the document body are extracted in document
+ order. Each table row becomes one "| cell | cell |" line, and a nested
+ table is flattened into its cell. Headers, footers, footnotes, and
+ content controls are not included.
"""
try:
from docx import Document
+ from docx.table import Table
except ImportError as e:
raise AirflowOptionalProviderFeatureException(e)
doc = Document(stream)
- paragraphs = [p.text for p in doc.paragraphs if p.text.strip()]
- text = "\n\n".join(paragraphs)
+ blocks = []
+ for block in doc.iter_inner_content():
+ if isinstance(block, Table):
+ text = "\n".join(f"| {' | '.join(cells)} |" for cells in
self._get_docx_table_rows(block))
+ else:
+ text = block.text
+ if text.strip():
+ blocks.append(text)
+ text = "\n\n".join(blocks)
return [{"text": text, "metadata": {}}]
+
+ def _get_docx_table_rows(self, table: Table) -> list[list[str]]:
+ rows = []
+ for row in table.rows:
+ cells: list[str] = []
+ previous_cell = None
+ for cell in row.cells:
+ # python-docx repeats the same _Cell object for every grid
column a
+ # horizontal merge spans. A vertical merge repeats on each row
it spans.
+ if cell is previous_cell:
+ continue
+ previous_cell = cell
+ cells.append(self._get_docx_cell_text(cell))
+ if any(cells):
+ rows.append(cells)
+ return rows
+
+ def _get_docx_cell_text(self, cell: _Cell) -> str:
+ from docx.table import Table
Review Comment:
`_parse_docx_stream` already imported `Table` inside its `ImportError`
guard, and this helper is only reachable from there, so this second
function-body import runs once per cell without adding anything. Could you pass
`Table` down, or make the two helpers module-level functions that take it as an
argument?
##########
providers/common/ai/docs/operators/document_loader.rst:
##########
@@ -73,14 +73,28 @@ Install the ``docx`` extra to parse Word documents via
pip install "apache-airflow-providers-common-ai[docx]"
-All non-empty paragraphs are concatenated into a single document per file.
+Paragraphs and tables in the document body are concatenated, in document
+order, into a single document per file. Empty paragraphs and table rows are
+skipped. Each table row becomes one line:
+
+.. code-block:: text
+
+ Quarterly results
+
+ | Region | Revenue |
+ | EMEA | 1.2M |
+
+A cell merged across columns appears once. A cell merged down several rows is
+repeated on each of those rows, so every row still reads on its own. A table
+nested inside a cell is flattened into that cell, with ``/`` between its cells
+and ``;`` between its rows.
.. note::
- DOCX extraction reads paragraph text only. Tables, headers, footers, and
- footnotes are not included. For richer DOCX parsing, use a dedicated
- extraction tool (``Unstructured``, ``docling``) as a custom parser
- backend.
+ Headers, footers, footnotes, and content controls are not included. For
Review Comment:
Now that content controls are listed too, this reads as a full list, but
text boxes and pending tracked insertions (`w:ins`) are also dropped: `Base ` +
an inserted run comes out as `'Base '`. That predates this PR, but maybe phrase
it positively instead? For example: "Only paragraphs and tables in the document
body are read. Headers, footers, footnotes, content controls, text boxes, and
pending tracked insertions are not included."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
