Skip to content

feat(tree): Update experimental Table Schema APIs to support adding additional properties to Column/Row Schema #24463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 105 commits into from
Apr 25, 2025

Conversation

Josmithr
Copy link
Contributor

@Josmithr Josmithr commented Apr 25, 2025

This PR also decouples Row and Column schema. Row previously took schema-specific Column as input so it could offer APIs that took in Column nodes. These have been replaced with 2 overloads each: 1 that takes an ID (resolving an existing TODO) and one that takes in an IColumn.

Notes:

  • Props schema are currently taken in the form of a node schema (ImplicitAllowedTypes). Preferably, they would be a field schema (ImplicitFieldSchema) so that users could make the property optional. I encountered some issues where TypeScript was generating invalid .d.ts files when I tried this previously. I will continue investigating and make this change in a subsequent PR.
  • Props schema are currently required. I will make them optional in a future PR. For now, if props are not desired, the user can specify SchemaFactory.null.

@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: tree area: examples Changes that focus on our examples labels Apr 25, 2025
@Josmithr Josmithr marked this pull request as ready for review April 25, 2025 18:23
@Copilot Copilot AI review requested due to automatic review settings April 25, 2025 18:24
@Josmithr Josmithr requested a review from a team as a code owner April 25, 2025 18:24
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the experimental Table Schema APIs to support additional properties for Column and Row schemas by introducing new props schemas and updating the tests and internal type signatures accordingly. Key changes include:

  • Introducing ColumnProps and RowProps classes for schema definitions.
  • Updating the createColumn, createRow, and createTable implementations and tests to require and handle a new “props” field.
  • Adjusting tests to consistently include a props object when creating schema instances.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
packages/dds/tree/src/test/tableSchema.spec.ts Updates to test cases to include props for columns and rows.
packages/dds/tree/src/tableSchema.ts Adjustments to schema creation functions and type signatures to support props.
examples/utils/import-testing/src/test/importer.spec.ts Updates to integration tests to utilize the new props-based API.
Comments suppressed due to low confidence (2)

packages/dds/tree/src/test/tableSchema.spec.ts:77

  • Consider adding tests for cases where the props field is omitted to ensure the schema behaves as expected when props are not provided, which will help ensure backward compatibility.
treeView.initialize(new Table({

packages/dds/tree/src/tableSchema.ts:470

  • Add inline documentation clarifying that using 'inputSchemaFactory.null' as the default for props is a temporary workaround, and consider extracting this logic into a helper function to improve readability.
const column = columnSchema ?? createColumn(inputSchemaFactory, inputSchemaFactory.null);

@@ -180,31 +202,34 @@ export namespace TableSchema {
const rowFields = {
id: schemaFactory.identifier,
cells: schemaFactory.map("Row.cells", cellSchema),
} as const satisfies Record<string, ImplicitFieldSchema>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discovered that the satisfies clause here was actually resulting in different (and incorrect) typing to be applied to rowFields. This appears to be a TypeScript bug. But since this extra check isn't really necessary for type-safety in this function, it can be safely removed.

@Josmithr Josmithr requested review from a team and CraigMacomber April 25, 2025 18:52
@Josmithr Josmithr enabled auto-merge (squash) April 25, 2025 21:44
@Josmithr Josmithr merged commit e43aef4 into microsoft:main Apr 25, 2025
35 checks passed
@Josmithr Josmithr deleted the tree/table-api-props branch April 25, 2025 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: tree area: dds Issues related to distributed data structures area: examples Changes that focus on our examples base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants