qid as number, number and cleanup fromArrow #197
Merged
Conversation
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
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| import { add_or_delete_column } from './Deeptable'; | ||
| import type * as DS from './types'; | ||
| import { extent } from 'd3-array'; | ||
| import { extent, extent, range } from 'd3-array'; |
There was a problem hiding this comment.
There's a duplicate import of extent in this line. It should be corrected to:
import { extent, range } from 'd3-array';This will eliminate the redundancy while maintaining all the needed functionality.
Suggested change
| import { extent, extent, range } from 'd3-array'; | |
| import { extent, range } from 'd3-array'; |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
src/Deeptable.ts
Outdated
| const tix = tileKey_to_tix((row as StructRowProxy)['key'] as string); | ||
| flatTree[tix] = row as StructRowProxy; | ||
| } | ||
| console.log({flatTree}) |
There was a problem hiding this comment.
This console.log({flatTree}) statement appears to be debugging code that was accidentally left in. Consider removing this before merging to keep the production code clean and avoid unnecessary console output.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to f777b04 in 2 minutes and 0 seconds. Click for details.
- Reviewed
338lines of code in10files - Skipped
0files when reviewing. - Skipped posting
5draft 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/Deeptable.ts:965
- Draft comment:
Consider optimizing the getQids tile lookup. Iterating over all tiles via this.map(...) can be inefficient—using a direct lookup (e.g. a dictionary keyed by tile index) would improve performance. - Reason this comment was not posted:
Confidence changes required:70%<= threshold70%None
2. src/tile.ts:136
- Draft comment:
Instead of setting a property to undefined for column removal (transformation_holder[colname] = undefined), consider using the delete operator to completely remove the key and free memory. - Reason this comment was not posted:
Confidence changes required:60%<= threshold70%None
3. src/regl_rendering.ts:553
- Draft comment:
Clarify in a comment that the 'color_pick' function returns a Qid tuple [tile_number, row_number] instead of a full row proxy. - Reason this comment was not posted:
Confidence changes required:50%<= threshold70%None
4. src/types.ts:111
- Draft comment:
Typo: Consider replacing "publically" with "publicly". - Reason this comment was not posted:
Comment was on unchanged code.
5. src/wrap_arrow.ts:17
- Draft comment:
There's a duplicate import for 'extent' on this line. Please remove one of the duplicates. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_2E5IpeuZO4MLHvNX
You can customize 
| import { add_or_delete_column } from './Deeptable'; | ||
| import type * as DS from './types'; | ||
| import { extent } from 'd3-array'; | ||
| import { extent, extent, range } from 'd3-array'; |
There was a problem hiding this comment.
Duplicate import: 'extent' is imported twice from 'd3-array'. Remove one instance to avoid confusion.
Suggested change
| import { extent, extent, range } from 'd3-array'; | |
| import { extent, range } from 'd3-array'; |
bmschmidt
force-pushed
the
05-25-remove_extraneous_tixrixqid_code
branch
from
be1bb51 to
30974f6
Compare
bmschmidt
force-pushed
the
05-26-qid_as_number_number_and_cleanup_fromarrow
branch
from
f777b04 to
01f7d5b
Compare
bmschmidt
force-pushed
the
05-25-remove_extraneous_tixrixqid_code
branch
from
30974f6 to
e9cc12d
Compare
bmschmidt
force-pushed
the
05-26-qid_as_number_number_and_cleanup_fromarrow
branch
from
01f7d5b to
8731b9d
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 8731b9d in 1 minute and 51 seconds. Click for details.
- Reviewed
338lines of code in10files - Skipped
0files when reviewing. - Skipped posting
7draft 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/wrap_arrow.ts:87
- Draft comment:
Using @ts-expect-error to suppress missing types for tableFromArrays is acceptable temporarily; consider adding a TODO comment to revisit this when Arrow JS type definitions are updated. - 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% While the typing issue is real, adding TODO comments about external library type definitions isn't very actionable. The team can't control when Arrow JS will update their types. The current @ts-expect-error comments are already descriptive about why they're needed. Adding TODO comments won't help and could become stale. The comment identifies a real issue with type definitions. Having a TODO could help track when this technical debt can be removed. TODOs about external dependencies are rarely actionable and often become stale. The existing @ts-expect-error comments already document the issue clearly. Delete the comment. The existing @ts-expect-error comments are sufficient documentation, and adding TODOs about external library updates isn't actionable.
2. tests/dataset.spec.js:64
- Draft comment:
The test assertion uses a logarithmic comparison to check selection ratios. Consider using a direct ratio (or relative difference) check for clarity. - 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. tests/dataset.spec.js:127
- Draft comment:
The use of negative indexing (e.g., sorted.get(-1)) for selecting the last element is clever; ensure that this behavior is clearly documented in the public API. - Reason this comment was not posted:
Confidence changes required:70%<= threshold70%None
4. src/regl_rendering.ts:551
- Draft comment:
The comment uses the word "complainy", which might be a typographical error or an informal term. Consider revising it to a more standard term if appropriate. - Reason this comment was not posted:
Comment was on unchanged code.
5. src/wrap_arrow.ts:17
- Draft comment:
There is a duplicate import of 'extent' in the statementimport { extent, extent, range } from 'd3-array';. Please remove the redundant 'extent' to avoid potential confusion. - Reason this comment was not posted:
Marked as duplicate.
6. src/wrap_arrow.ts:40
- Draft comment:
Typo: There is an extra space before the colon in the type annotation. Consider changingconst extents : string[] = []toconst extents: string[] = []. - 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% While the comment is technically correct, it's an extremely minor style issue that doesn't affect functionality. Our rules emphasize focusing on substantive issues and code quality problems. This kind of minor formatting issue would typically be handled by a linter or formatter rather than manual review comments. The comment is factually correct and points out a real inconsistency in code style. Consistent formatting can make code more readable. While code style consistency is good, this is too minor an issue to warrant a PR comment. It would be better handled by automated tooling like Prettier or ESLint. Delete this comment as it's too minor an issue and doesn't align with our focus on substantive code quality issues.
7. src/wrap_arrow.ts:53
- Draft comment:
Minor issue: There appears to be extra trailing whitespace at the end of this line. Please remove the extra spaces aftervectorFromArray(array));. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =60%<= threshold70%The comment is suggesting a minor formatting change, which is not strictly necessary for functionality or performance. It doesn't violate any specific rules, but it doesn't add significant value either. However, it could be considered a good practice to maintain consistent code formatting.
Workflow ID: wflow_br11qmRfdh9yvnz1
You can customize
| import { add_or_delete_column } from './Deeptable'; | ||
| import type * as DS from './types'; | ||
| import { extent } from 'd3-array'; | ||
| import { extent, extent, range } from 'd3-array'; |
There was a problem hiding this comment.
Duplicate import of extent from d3-array. Remove the extra extent to keep imports DRY.
Suggested change
| import { extent, extent, range } from 'd3-array'; | |
| import { extent, range } from 'd3-array'; |
| import { add_or_delete_column } from './Deeptable'; | ||
| import type * as DS from './types'; | ||
| import { extent } from 'd3-array'; | ||
| import { extent, extent, range } from 'd3-array'; |
There was a problem hiding this comment.
Fix duplicate import of 'extent' from 'd3-array' by removing one instance: change to 'import { extent, range } from 'd3-array';'
Spotted by Diamond (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| Timestamp, | ||
| Utf8, | ||
| Vector, | ||
| TypeMap, |
There was a problem hiding this comment.
Remove unused 'TypeMap' import to clean up the code and avoid potential issues
Spotted by Diamond (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
bmschmidt
force-pushed
the
05-26-qid_as_number_number_and_cleanup_fromarrow
branch
from
8731b9d to
62ea242
Compare
| import { add_or_delete_column } from './Deeptable'; | ||
| import type * as DS from './types'; | ||
| import { extent } from 'd3-array'; | ||
| import { extent, extent, range } from 'd3-array'; |
There was a problem hiding this comment.
Remove the duplicate 'extent' import. Change 'import { extent, extent, range } from 'd3-array';' to 'import { extent, range } from 'd3-array';'
Spotted by Diamond (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| Timestamp, | ||
| Utf8, | ||
| Vector, | ||
| TypeMap, |
There was a problem hiding this comment.
Remove the unused 'TypeMap' import if it's not needed elsewhere in the code
Spotted by Diamond (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 62ea242 in 2 minutes and 3 seconds. Click for details.
- Reviewed
331lines of code in10files - Skipped
0files when reviewing. - Skipped posting
7draft 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/tixrixqid.ts:109
- Draft comment:
The inverse function (tixToZxy) uses a logarithm–based approach that may be subject to floating–point rounding errors at very high tile indices. Consider adding a comment or safeguards if large values are expected. - Reason this comment was not posted:
Confidence changes required:66%<= threshold70%None
2. src/wrap_arrow.ts:31
- Draft comment:
Consider renaming variable 'tb' to a more descriptive name (e.g. 'arrowTable') for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold70%None
3. tests/dataset.spec.js:64
- Draft comment:
The use of logarithmic comparisons in selection size assertions is non‐obvious. Adding comments to explain the rationale behind using log ratios would improve maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold70%None
4. src/tile.ts:129
- Draft comment:
Deleting columns via deleteColumn only logs a warning and resets internal cache. Consider whether additional cleanup (e.g. GPU resource release) is needed to prevent stale data or memory leaks. - Reason this comment was not posted:
Confidence changes required:50%<= threshold70%None
5. src/types.ts:50
- Draft comment:
Typographical suggestion: If referring to Apache Arrow, consider capitalizing 'arrow' to 'Arrow' in this comment. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold70%This comment is purely informative and suggests a typographical change in a comment, which is not critical to the functionality or performance of the code. It doesn't address any of the specific rules or guidelines provided, such as DRY, memory leaks, or performance issues. Therefore, it should be removed.
6. src/types.ts:111
- Draft comment:
Typo: Consider changing "publically" to "publicly" in this comment. - Reason this comment was not posted:
Comment was on unchanged code.
7. src/wrap_arrow.ts:17
- Draft comment:
There appears to be a duplicate import of 'extent' in the import statement from 'd3-array'. Consider removing one of the 'extent' references if it's not intentional. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_t7t0bHNFdOscdTOD
You can customize
| import { add_or_delete_column } from './Deeptable'; | ||
| import type * as DS from './types'; | ||
| import { extent } from 'd3-array'; | ||
| import { extent, extent, range } from 'd3-array'; |
There was a problem hiding this comment.
Duplicate import of extent from d3-array should be removed.
Suggested change
| import { extent, extent, range } from 'd3-array'; | |
| import { extent, range } from 'd3-array'; |
Collaborator
Author
Merge activity
|
bmschmidt
changed the base branch from
05-25-remove_extraneous_tixrixqid_code
to
graphite-base/197
bmschmidt
force-pushed
the
05-26-qid_as_number_number_and_cleanup_fromarrow
branch
from
62ea242 to
7d2be7f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Two related goals in this one.
Important
This PR updates QID handling to use consistent tuples and enhances manifest loading for Arrow tiles, affecting several core functions and tests.
Qidis now consistently a tuple[number, number]instead of[number, Some<number>].getQids()inDeeptable.tsretrievesStructRowProxyfor QID tuples.color_pick()inregl_rendering.tsreturns QID tuples.set_highlit_points()ininteraction.tsuses QID tuples.max_pointsandsource_urlinFourClasses.svelte.manifestwithmetadatainSelectPoints.svelteanddataset.spec.js.This description was created by
for 62ea242. You can customize this summary. It will automatically update as commits are pushed.