Conversation
comparePKComponent compared two numbers by value, but a number and a string as strings. That order is not transitive: "1a" < "9" < "10", but "10" < "1a". The keys come from map iteration, so sort.Slice put rows with such keys in a different order on each run. Numbers now come before all other strings. NaN and the infinities count as text, because NaN is not equal even to itself.
table-diff --output html was killed by the OOM killer on a diff of 493,200 rows, while the JSON report for the same diff was written without trouble. The HTML writer held the whole report in memory at once: a json.Marshal copy of the full diff for the page script, a struct with escaped cells for every row, and the rendered page in one bytes.Buffer. Each row takes several kilobytes of markup, so the buffer alone grew to gigabytes. The template is now split into blocks: page head, pair head, one block per row, pair tail and page tail. The writer runs them one by one into a buffered writer on the report file, and builds the data of a row only while that row is written. Only the row keys of the whole diff stay in memory. If writing fails, the half-written file is removed, and the error names the JSON report, which is complete. The page script no longer gets a copy of the whole diff. It gets one entry per row, with the pair, the kind of difference, and each primary key value as the JSON text of the diff file, and it builds the repair plan from that. The script copies the key text into the YAML as it is. Before, it read keys as JavaScript numbers, so a bigint above 2^53 could name the next row, and a text key such as "007" became 7 and matched no row. Range rules are used only when every key of the diff is a whole number. On a synthetic diff of 493,200 rows, peak memory fell from 4.8 GB to 0.55 GB; the diff itself takes 0.2 GB. The markup of the page does not change. Tests: the rows on the page match the embedded rows for each pair; key literals; the output is the same from run to run; a write error in each part of the page is returned and leaves no file. Three tests run the page script in Node.js (they skip without node) and resolve the plan with the repair executor.
Streaming keeps table-diff alive, but a report of hundreds of thousands
of rows is still gigabytes of markup that no browser can open. The HTML
report now shows at most DefaultMaxHTMLRows (10,000) rows for each node
pair. A row is one primary key: a value difference or a row missing on
one node. Rows are taken in report order: value differences, then rows
missing on the second node, then rows missing on the first. The JSON
report does not change and always has every row.
A truncated report says so: a banner at the top, a summary item, the
count in the section header ("25 of 53 rows shown"), a note under the
section toolbar with the hidden rows by kind, and a last table row. In a
truncated section the bulk controls say "all 25 shown rows", and they
come after the notes.
The repair plan needs care here. table-repair applies the plan's
default_action to every row of the diff file that no rule matches, and
that includes the rows the page did not show. With the old default
keep_n1, a hidden row missing on n1 made table-repair reject the whole
plan, and hidden value differences silently got keep_n1. So a plan built
on a truncated page now has default_action: skip and an explicit rule
for every shown row, and the YAML starts with a comment that says so and
how to cover all rows. A complete report gives the same plan as before.
Known limit: plan rules match by key and kind of difference, not by node
pair. With three or more nodes, a rule for a key shown in one pair also
acts on that key where it is hidden in another pair.
Every count in a section now comes from the rows the report renders. If
table-diff counted rows that show no visible difference (1 against
"1"), a note says how many. The row action default is labelled with the
action it takes, for example "Default: insert from n1".
WriteDiffReport takes the limit as a new argument. Callers pass 0 (the
default) until the next commit adds the setting.
Tests: truncation in value differences and in missing rows, the counts,
the plan data of a truncated page, and a Node.js test that a plan from a
truncated report resolves and changes only the shown rows.
The HTML report limit was fixed at the default. It can now be set: - table_diff.max_html_rows and mtree.diff.max_html_rows in ace.yaml; - --max-html-rows on table-diff and mtree table-diff; - max_html_rows in the args of a scheduled table-diff job. The command-line value wins, then the config value, then the default of 10,000 rows per node pair. 0 means "not set". There is no setting for "no limit"; a large number does the same. A negative value is an error in every place, where before a negative config value would have been ignored without a word. repset-diff and schema-diff take the value from table_diff in the config. The sample and default configs list the key. Tests: how TableDiffTask.Validate resolves the value, and the errors for negative values.
The table-diff and mtree table-diff docs describe --max-html-rows and the max_html_rows keys, what a truncated report shows, and what a repair plan built on a truncated page does: rules only for the shown rows and default_action: skip for the rest. They also state the known limit for three or more nodes, where a plan rule acts on a key in every node pair. The CHANGELOG lists the OOM fix, the exact keys in repair plans, and the stable key order.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 37 minutes. View limit detailsLimit details: You’ve used the included review currently available. Only developers with an assigned seat can start an on-demand review using credits. Ask an admin to assign your seat or change the review continuation mode in Billing. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (20)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAdds configurable per-node-pair row limits for HTML diff reports. The HTML writer streams report output, while JSON reports retain every row. Repair plans from truncated reports skip hidden rows by default, and plan generation preserves primary-key values as represented in the diff data. ChangesHTML Diff Reports
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to This change streams HTML diff reports to disk and caps the rows shown per node pair, with a configurable limit. JSON reports still contain every row. Repair plans built from a truncated report skip hidden rows by default, and primary keys keep their exact values. No outstanding defects were identified, and the change appears ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 53.13% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 64 functions across 13 files. (7 skipped: 7 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
A rabbit sees the rows stream by, Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 52 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codacy flagged five functions of the new report code as too complex (cyclomatic complexity above 8, or more than 50 lines). The behaviour does not change: for the same diff, the page markup and the embedded data are the same bytes as before. - renderHTMLDiffReport and writeHTMLPair had an error check after every template block. A small htmlBlockWriter now keeps the first error and skips the rest, so one check at the end is enough. The pair plans, the page head and the report info are built in their own functions, and the value rows and the missing rows of a pair are written by two. - buildHTMLPairPlan: finding the node names, indexing the rows of one node, and sorting the rows into their lists are now three helpers. - writeHTMLDiffData and buildHTMLSummaryItems got one helper each, to stay under the same limits. - In the page script, collectRows and buildPKMatchers each give part of their work to a helper: picking the rows once per key, the default action of a row, and the ranges over whole numbers. One error message changes: a failure in any part of the page now reads "failed to write HTML diff report".
Codacy reported "Non-HTML variable 'report' is used to store raw HTML" for `const report = diff.html_report`. The rule judges by names only: a property whose name has "html" in it is taken to hold markup. This one holds JSON (truncated, integer_pk, pairs, diff_file), and the page script only reads its fields into the YAML text of the repair plan. The script never writes markup into the page. Call the key report_info, so the check has nothing to match.
danolivo
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
table-diff --output htmlwas killed by the OOM killer on a diff of493,200 rows; the JSON report for the same diff was fine. The HTML writer
built the whole report in memory: a JSON copy of the full diff for the page
script, a struct for every row, and the rendered page in one buffer. With
several kilobytes of markup per row, that needs gigabytes. A page of that
size could not be opened in a browser anyway.
What this PR does
(page head, pair head, one block per row, pair tail, page tail). Each row
is written to a buffered file writer and then dropped. Only the row keys
of the whole diff stay in memory. If writing fails, the partial file is
removed, and the error names the JSON report, which is complete.
max_html_rowsrows per node pair (default 10,000).Rows are taken in report order: value differences, then rows missing on
the second node, then rows missing on the first. A truncated report says
so in a banner at the top, in the summary, in the section header ("25 of
53 rows shown"), in a note that lists the hidden rows by kind, and in the
last row of the table. The JSON report does not change and always has
every row.
table-repairappliesthe plan's
default_actionto every row of the diff file that no rulematches, including the rows the page did not show. With the old default
keep_n1, a hidden row missing on n1 madetable-repairreject the wholeplan. A plan from a truncated page now has
default_action: skipand anexplicit rule for every shown row. The YAML starts with a comment that
explains this and how to cover all rows. A plan from a complete report is
the same as before.
keys as JavaScript numbers, so a bigint above 2^53 could name the next
row, and a text key such as
"007"became7and matched nothing. Thepage now embeds each key as the JSON text of the diff file, and the script
copies it into the YAML as it is. Range rules are used only when every key
of the diff is a whole number.
sorted in a different order on each run, because the comparison was not
transitive. Numbers now come before text.
Settings
ace.yamltable_diff.max_html_rows,mtree.diff.max_html_rows--max-html-rowsontable-diffandmtree table-diffmax_html_rows(table-diff jobs)The command-line value wins, then the config value, then the default of
10,000.
0means "not set". A negative value is an error.repset-diffand
schema-diffusetable_diff.max_html_rows.