Skip to content
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

feat(nodejs): feature parity [1/N] - remote table #1378

Merged
merged 12 commits into from
Jun 17, 2024

Conversation

universalmind303
Copy link
Contributor

closes #1362

@universalmind303 universalmind303 requested review from westonpace and wjones127 and removed request for westonpace June 11, 2024 17:24
@github-actions github-actions bot added the enhancement New feature or request label Jun 11, 2024
@universalmind303 universalmind303 changed the title feat(nodejs): remote table feat(nodejs): Feature Parity [1/N] remote table Jun 17, 2024
Copy link

ACTION NEEDED

Lance follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

For details on the error please inspect the "PR Title Check" action.

@universalmind303 universalmind303 changed the title feat(nodejs): Feature Parity [1/N] remote table feat(nodejs): feature parity [1/N] - remote table Jun 17, 2024
Copy link
Contributor

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

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

Good work! I have a couple minor comments.

Comment on lines 113 to 115
if (options?.mode) {
console.warn(`mode is not supported in remote connection, ignoring.`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you describe what mode behaviour it is following here? It it going to error if the table already exists? Or overwrite the table?

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 can definitely make this a bit more informative. FWIW , I was copying the python message

I'd assume cloud behavior is to error on exists?

Copy link
Contributor

Choose a reason for hiding this comment

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

Default behavior looks like it will error if the table already exists. We should make sure we have an integration test that validates this.

Comment on lines +62 to +63
// TODO: parse this into a valid arrow schema
return resp.schema;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this handled in one of the follow-up PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have this handled yet.

The old vectorDB sdk was returning this AS IS and silently deviating from the type hints. Currently this mimics the same functionality, but I wanted to call out that this is not the correct behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. This is fine for now.

@universalmind303 universalmind303 merged commit e7022b9 into lancedb:main Jun 17, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for "Remote" connections
2 participants