qid refactors

[bmschmidt]

Modify the various handler (scatterplot.tooltip_handler, scatterplot.click_handler ), functions so that the underlying type is from a Qid, not a structrowproxy.
For most of the setter APIs that populate these e.g. (scatterplot.tooltip_html) retain the current api that allows setting with a function structRow proxy, and just pipe that through a conversion function; while changing internal code to go into the internals and pass qids directly.
Remove PointMouseoverFunction, which is now replaced by handle_highlit_point_change. (When you mouseover a point, it sets it to highlighted by adding the svg circle).
Change handle_highlit_point_change to work directly on Quadtree ids.
Unrelated fix lost in a recent update: Add a promise await for all data points on the root tile before attempting to plot, because without it we can't determine arrow types sufficiently to use.

---

Important

Refactor scatterplot handlers to use Qid instead of StructRowProxy and ensure data points are loaded before plotting.

• Behavior:
  • Refactor scatterplot.tooltip_handler, scatterplot.click_handler, and scatterplot.handle_highlit_point_change to use Qid instead of StructRowProxy.
  • Retain existing API for setting handlers with StructRowProxy by converting to Qid internally.
  • Remove PointMouseoverFunction, replaced by handle_highlit_point_change.
  • Add promise await for data points on root tile before plotting.
• Functions:
  • Update set_highlit_points() in interaction.ts to use Qid.
  • Modify tooltip_html and click_function setters in scatterplot.ts to convert StructRowProxy to Qid.
• Misc:
  • Remove console logs from integers.html.
  • Minor whitespace change in ColorAesthetic.ts.

^{This description was created by for 76ea167. You can customize this summary. It will automatically update as commits are pushed.}

[bmschmidt]

• qid refactors #198 👈 (View in Graphite)
• cleaner selection get methods #199
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[bmschmidt]

1. Modify the various handler (scatterplot.tooltip_handler, scatterplot.click_handler ), functions so that the underlying type is from a Qid, not a structrowproxy.
2. For most of the setter APIs that populate these e.g. (scatterplot.tooltip_html) retain the current api that allows setting with a function structRow proxy, and just pipe that through a conversion function; while changing internal code to go into the internals and pass qids directly.
3. Remove PointMouseoverFunction, which is now replaced by handle_highlit_point_change. (When you mouseover a point, it sets it to highlighted by adding the svg circle).
4. Change handle_highlit_point_change to work directly on Quadtree ids.

Unrelated fix lost in a recent update: Add a promise await for all data points on the root tile before attempting to plot, because without it we can't determine arrow types sufficiently to use.

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to f253dd3 in 2 minutes and 24 seconds. Click for details.

• Reviewed 315 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 7 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src/scatterplot.ts:586

• Draft comment:
  Potential runtime error: getQids([e]) may return an empty array. Consider checking that the returned array is non-empty before accessing [0].
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 70% This is a setter for tooltip_html that wraps a user-provided function. The code assumes getQids() will always return a valid array with at least one element. Looking at the type signature and usage, 'e' is a Qid that came from the system itself, not user input. Since this is an internal conversion between the system's own ID types, getQids() should always return a valid result for a valid Qid input. The array access is likely safe in practice. I could be wrong about the internal guarantees - perhaps there are edge cases where a Qid becomes invalid or getQids() could return an empty array. The type system doesn't enforce non-emptiness. While type safety would be nice, this is an internal conversion between the system's own ID types that should be reliable by design. Adding runtime checks would add overhead to a performance-critical tooltip handler. The comment raises a theoretically valid concern but the array access is likely safe in practice since it's dealing with internal ID conversions. The performance cost of additional checks outweighs the theoretical safety benefit.

2. src/scatterplot.ts:593

• Draft comment:
  Similarly, in the click_function setter, verify that getQids([e]) returns at least one element before using [0] to avoid passing undefined.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 70% The comment raises a valid concern about array access safety. However, looking at the broader context, getQids is being called with a single element array [e], so it should always return at least one element. The code is following an established pattern also used in tooltip_html. This seems to be an intentional design where getQids is expected to always return an array with at least one element when given a single input. I could be wrong about getQids' behavior - without seeing its implementation, I can't be 100% certain it always returns a non-empty array. While true that I can't see getQids' implementation, the consistent usage pattern across the codebase and the fact that it's part of the core functionality suggests this is expected behavior rather than a bug. The comment should be deleted - it appears to be questioning an intentional design pattern rather than identifying a real issue.

3. src/tile.ts:136

• Draft comment:
  Instead of setting a property to undefined in transformation_holder, consider using the 'delete' operator to properly remove the property and avoid lingering keys.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. src/tile.ts:420

• Draft comment:
  The arrowFetchCalls cache is never evicted, which might lead to memory buildup with many fetches. Consider an eviction strategy if tile fetches are numerous.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. src/tile.ts:557

• Draft comment:
  Avoid throwing a string as an error. Use an Error object for better debugging and consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. src/tile.ts:681

• Draft comment:
  The descendant_cache is never invalidated. Ensure that this caching strategy is appropriate for dynamic tile structures or document its limitations.
• Reason this comment was not posted:
  Confidence changes required: 70% <= threshold 70% None

7. src/scatterplot.ts:909

• Draft comment:
  Typo: Consider renaming "ChangeToHighlitPointFunction" to "ChangeToHighlightPointFunction" (or "ChangeToHighlightedPointFunction") to avoid potential confusion.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_GKOM0HzuORcZdMlZ

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed c81041e in 1 minute and 57 seconds. Click for details.

• Reviewed 336 lines of code in 6 files
• Skipped 0 files when reviewing.
• Skipped posting 8 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src/tile.ts:130

• Draft comment:
  In deleteColumn(), rather than setting the key in transformation_holder to undefined, use the delete operator (e.g. delete this.transformation_holder[colname]) to avoid retaining keys and potential memory bloat.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. src/tile.ts:331

• Draft comment:
  The getter for record_batch falls back to creating a new RecordBatch using a dummy '__null' vector if _batch isn’t set. This fallback may mask issues with missing data; ensure that relying on this logic is safe and doesn’t incur unnecessary overhead.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. src/tile.ts:539

• Draft comment:
  The manifest derivation in deriveManifestInfoFromTileMetadata sometimes stores the extent as a JSON string (via JSON.stringify) and later parses it. Consider storing the extent consistently as an object to simplify downstream usage.
• Reason this comment was not posted:
  Confidence changes required: 70% <= threshold 70% None

4. src/tile.ts:411

• Draft comment:
  In get_arrow(), the check for 'if (suffix === undefined)' may be misleading since the parameter type allows null. Verify that passing null (the intended default) is handled correctly and consider clarifying the check.
• Reason this comment was not posted:
  Confidence changes required: 70% <= threshold 70% None

5. src/tile.ts:681

• Draft comment:
  The macrotile_descendants() function and its helper logic are quite complex and currently lack detailed inline commentary. Adding more descriptive comments about the algorithm and intended output would improve maintainability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. src/tile.ts:420

• Draft comment:
  The arrowFetchCalls cache is never cleared. If many suffixes are used over time, this could lead to memory accumulation. Consider implementing a cache eviction strategy.
• Reason this comment was not posted:
  Confidence changes required: 60% <= threshold 70% None

7. src/interaction.ts:25

• Draft comment:
  Typographical error: The new property 'qid: Qid' is missing a semicolon at the end. Please add a semicolon to maintain consistency with the other property declarations.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 70% This is an extremely minor style issue. The code will work perfectly fine either way. Most teams use automated formatting tools like Prettier that would fix this automatically. The comment doesn't point out any real issues with the code's functionality or design. The code style should be consistent. Having inconsistent semicolon usage could indicate sloppiness. While consistency is good, this is exactly the kind of minor formatting issue that should be handled by automated tools, not code review comments. Code review should focus on more substantive issues. Delete this comment. It's too minor of an issue and would be better handled by automated formatting tools.

8. src/scatterplot.ts:909

• Draft comment:
  Typo: The function name "ChangeToHighlitPointFunction" may be intended to be "ChangeToHighlightPointFunction". Consider renaming for clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_RN65zjLmxCXpY6D5

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed 76ea167 in 2 minutes and 11 seconds. Click for details.

• Reviewed 362 lines of code in 6 files
• Skipped 0 files when reviewing.
• Skipped posting 8 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. src/scatterplot.ts:584

• Draft comment:
  In the tooltip_html setter, the callback now calls 'deeptable.getQids([e])[0]'. Please add a check to ensure that getQids returns a non‐empty array before indexing, to avoid runtime errors if no Qid is found.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 70% The comment is about a real code change in the diff. However, without seeing the implementation of getQids(), I can't be certain whether it could return an empty array. The code seems to be part of a refactor to use Qids instead of StructRowProxy objects. The comment is speculative - it assumes getQids() might fail without evidence. Following our rules, we should not make speculative comments. I could be wrong about getQids() - maybe it really can return empty arrays in some edge cases. The code would crash in that case. While array access could theoretically fail, we need strong evidence that getQids() can actually return empty arrays before suggesting defensive coding. Without that evidence, this is a speculative comment. Delete the comment because it makes a speculative suggestion without clear evidence that getQids() can return empty arrays.

2. src/scatterplot.ts:591

• Draft comment:
  Similarly, in the click_function setter the conversion via getQids([e])[0] assumes a valid Qid exists. Consider validating the returned array before using its first element.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 70% The comment points out a potential edge case where getQids() could return an empty array, which would make [0] return undefined. However, looking at the type signature and usage, 'e' is a Qid that was already validated earlier in the call chain. The click handler would only be called with a valid Qid from an actual click event. The suggested validation would add unnecessary defensive programming. I could be wrong about the validation happening earlier in the call chain. There could be edge cases where an invalid Qid makes it through. Even if there are edge cases, adding validation here would be the wrong place - we should validate at the boundaries where Qids enter the system, not in every internal conversion function. Delete the comment. The suggested validation would add unnecessary defensive programming in an internal conversion function where the input types are already constrained.

3. src/selection.ts:1397

• Draft comment:
  The quickSelect algorithm here is quite complex. Consider adding more inline comments to explain the partitioning and recursion steps for future maintainers.
• Reason this comment was not posted:
  Confidence changes required: 60% <= threshold 70% None

4. src/tile.ts:412

• Draft comment:
  The get_arrow function immediately throws if 'suffix' is undefined. It might be clearer to enforce using null explicitly rather than undefined, to improve API clarity.
• Reason this comment was not posted:
  Confidence changes required: 70% <= threshold 70% None

5. src/tile.ts:560

• Draft comment:
  In deriveManifestInfoFromTileMetadata, if min_ix > max_ix the code recovers by resetting values. It would be good to add more logging or error handling here to avoid silent recovery of corrupted metadata.
• Reason this comment was not posted:
  Confidence changes required: 70% <= threshold 70% None

6. src/tile.ts:665

• Draft comment:
  The macrotile and macrotile_siblings functions cache descendant keys in a Map. Ensure that this caching strategy won’t lead to unbounded memory growth over long sessions.
• Reason this comment was not posted:
  Confidence changes required: 60% <= threshold 70% None

7. src/interaction.ts:25

• Draft comment:
  Typographical: It looks like the 'qid' property is missing a semicolon at the end for consistency with the other properties in the type definition. Consider adding a semicolon after 'qid: Qid'.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 70% While the comment is technically correct about inconsistency in semicolon usage, this is a very minor style issue that would likely be caught by linting or formatting tools. The code will work perfectly fine either way. The rules state not to make comments that are obvious or unimportant, and this seems to fall into that category. The semicolon inconsistency could indicate broader style inconsistencies in the codebase that should be addressed. TypeScript best practices generally recommend consistent semicolon usage. While consistent style is good, this is too minor an issue to warrant a PR comment. This kind of formatting should be handled automatically by tools like Prettier or ESLint, not through manual code review comments. Delete this comment as it's too minor and would be better handled by automated formatting tools.

8. src/scatterplot.ts:909

• Draft comment:
  Typo: The class name 'ChangeToHighlitPointFunction' appears to have a misspelling. Consider renaming it to 'ChangeToHighlightPointFunction' for clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_bhj5ZWF2bZZgutsV

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

In moveCursorToQid, const indexMatch = which.indexOf(rowNumber) might return -1 if the row isn’t found. Please add error handling or a guard to avoid setting a negative cursor.

[graphite-app]

@bmschmidt applied this fix suggested by Graphite:

Suggested change

	const indexMatch = which.indexOf(rowNumber);
	const indexMatch = which.indexOf(rowNumber);
	if (indexMatch === -1) {
	throw new Error(`Row ${rowNumber} not found in selection`);
	}

[RLesser]

LGTM

[bmschmidt]

Merge activity

• May 29, 5:46 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• May 29, 5:47 PM UTC: Graphite rebased this pull request as part of a merge.
• May 29, 5:49 PM UTC: @bmschmidt merged this pull request with Graphite.