Skip to content

feat: Add BaseVector::createFromVariants convenience API#15955

Closed
PingLiuPing wants to merge 1 commit intofacebookincubator:mainfrom
PingLiuPing:lp_add_vector_creator
Closed

feat: Add BaseVector::createFromVariants convenience API#15955
PingLiuPing wants to merge 1 commit intofacebookincubator:mainfrom
PingLiuPing:lp_add_vector_creator

Conversation

@PingLiuPing
Copy link
Copy Markdown
Collaborator

@PingLiuPing PingLiuPing commented Jan 9, 2026

Add a new helper function to create a vector from a list of Variant values.
Supports all primitive types and complex types (ARRAY, MAP, ROW).

@PingLiuPing PingLiuPing requested a review from mbasmanova January 9, 2026 14:08
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 9, 2026
@netlify
Copy link
Copy Markdown

netlify Bot commented Jan 9, 2026 •
edited
Loading

✅ Deploy Preview for meta-velox ready!

Name Link
🔨 Latest commit aaf873d
🔍 Latest deploy log https://app.netlify.com/projects/meta-velox/deploys/6966815805c8f000084220ae
😎 Deploy Preview https://deploy-preview-15955--meta-velox.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@PingLiuPing PingLiuPing mentioned this pull request Jan 9, 2026
Closed
@PingLiuPing PingLiuPing force-pushed the lp_add_vector_creator branch from bb05df6 to 78bead1 Compare January 9, 2026 14:30
mbasmanova
mbasmanova reviewed Jan 9, 2026
View reviewed changes
Comment thread velox/vector/BaseVector.h Outdated
Comment on lines +607 to +609
/// Creates a vector from a list of Variant values.
/// Supports all primitive types and complex types (ARRAY, MAP, ROW).
static VectorPtr createFromConstants(
Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova Jan 9, 2026

Choose a reason for hiding this comment

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

createFromConstants

Let's rename to createFromVariants, add toVariants getter and move next to variantAt. Let's clarify that the result is a flat vector.

Variant variantAt(vector_size_t index) const;

std::vector<Variant> toVariants() const;

static VectorPtr createFromVariants(...);

Supports all primitive types and complex types (ARRAY, MAP, ROW).

This is redundant since it effectively means that the API supports all types.

Copy link
Copy Markdown
Collaborator Author

@PingLiuPing PingLiuPing Jan 9, 2026

Choose a reason for hiding this comment

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

Thanks.

mbasmanova
mbasmanova reviewed Jan 9, 2026
View reviewed changes
Comment thread velox/vector/tests/VariantToVectorTest.cpp Outdated
auto vector = BaseVector::createFromConstants(type, values, pool());

EXPECT_FALSE(vector->isConstantEncoding());
EXPECT_EQ(vector->type()->toString(), type->toString());
Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova Jan 9, 2026

Choose a reason for hiding this comment

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

use VELOX_EXPECT_EQ_TYPES

Copy link
Copy Markdown
Collaborator Author

@PingLiuPing PingLiuPing Jan 9, 2026

Choose a reason for hiding this comment

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

Thanks.

mbasmanova
mbasmanova reviewed Jan 9, 2026
View reviewed changes
Comment thread velox/vector/tests/VariantToVectorTest.cpp Outdated
void testFromConstants(
const TypePtr& type,
const std::vector<Variant>& values,
const VectorPtr& expected) {
Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova Jan 9, 2026

Choose a reason for hiding this comment

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

const VectorPtr& expected

Seems redundant to provide 'expected' as we can simply verify that values[i] == vector->variantAt(i).

Copy link
Copy Markdown
Collaborator Author

@PingLiuPing PingLiuPing Jan 9, 2026

Choose a reason for hiding this comment

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

Thanks, you are right.

mbasmanova
mbasmanova reviewed Jan 9, 2026
View reviewed changes
Comment thread velox/vector/tests/VariantToVectorTest.cpp Outdated
}));
}

TEST_F(VariantToVectorTest, createFromConstantsEmpty) {
Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova Jan 9, 2026

Choose a reason for hiding this comment

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

Perhaps, add some negative tests where variants do not have consistent types or do not match the 'type'.

Copy link
Copy Markdown
Collaborator Author

@PingLiuPing PingLiuPing Jan 9, 2026

Choose a reason for hiding this comment

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

Thanks, added a case to test type mismatch scenario.

mbasmanova
mbasmanova reviewed Jan 9, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

Curious, where do you want to use this new API.

@PingLiuPing PingLiuPing force-pushed the lp_add_vector_creator branch from 78bead1 to 20863af Compare January 9, 2026 21:04
@PingLiuPing PingLiuPing changed the title misc: Add createFromConstants helper function misc: Add createFromVariants helper function Jan 9, 2026
@mbasmanova mbasmanova changed the title misc: Add createFromVariants helper function feat: Add BaseVector::createFromVariants convenience API Jan 10, 2026
mbasmanova
mbasmanova reviewed Jan 10, 2026
View reviewed changes
Comment thread velox/vector/tests/VariantToVectorTest.cpp Outdated
};
EXPECT_THROW(
BaseVector::createFromVariants(VARCHAR(), values, pool()),
std::invalid_argument);
Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova Jan 10, 2026

Choose a reason for hiding this comment

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

Can we make sure to throw VeloxException? and use VELOX_ASSERT_THROW to verify the message?

Copy link
Copy Markdown
Collaborator Author

@PingLiuPing PingLiuPing Jan 12, 2026

Choose a reason for hiding this comment

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

Yes, this require some additional change to Variant::throwCheckIsKindError. And I also updatedVariant::throwCheckPtrError. @mbasmanova

mbasmanova
mbasmanova reviewed Jan 10, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

Looks great % small comment re: the exception.

@PingLiuPing PingLiuPing force-pushed the lp_add_vector_creator branch 2 times, most recently from 9b5fd3d to 0bc392c Compare January 12, 2026 11:25
mbasmanova
mbasmanova approved these changes Jan 12, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

Thanks.

@mbasmanova mbasmanova added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Jan 12, 2026
@kevinwilfong
Copy link
Copy Markdown
Contributor

kevinwilfong commented Jan 12, 2026

@PingLiuPing it looks like VariantOpaqueTest.opaque is expecting the exception to be of a certain type and is now failing, could you take a look?

@PingLiuPing
Copy link
Copy Markdown
Collaborator Author

PingLiuPing commented Jan 12, 2026

@PingLiuPing it looks like VariantOpaqueTest.opaque is expecting the exception to be of a certain type and is now failing, could you take a look?

@kevinwilfong Thank you, will check and update.

@PingLiuPing PingLiuPing force-pushed the lp_add_vector_creator branch from 0bc392c to 7360ab4 Compare January 12, 2026 16:52
@PingLiuPing
Copy link
Copy Markdown
Collaborator Author

PingLiuPing commented Jan 12, 2026

@kevinwilfong Fixed.

@meta-codesync
Copy link
Copy Markdown

meta-codesync Bot commented Jan 12, 2026

@kevinwilfong has imported this pull request. If you are a Meta employee, you can view this in D90518495.

@kevinwilfong
Copy link
Copy Markdown
Contributor

kevinwilfong commented Jan 13, 2026

@PingLiuPing could I get you to rebase this one as well on top of #15981 (again, thank you so much for that fix!)

@PingLiuPing PingLiuPing force-pushed the lp_add_vector_creator branch from 7360ab4 to aaf873d Compare January 13, 2026 17:31
@meta-codesync meta-codesync Bot closed this in d2c8304 Jan 15, 2026
@meta-codesync
Copy link
Copy Markdown

meta-codesync Bot commented Jan 15, 2026

@kevinwilfong merged this pull request in d2c8304.

@PingLiuPing PingLiuPing deleted the lp_add_vector_creator branch March 21, 2026 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mbasmanova mbasmanova mbasmanova approved these changes

@bikramSingh91 bikramSingh91 Awaiting requested review from bikramSingh91

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@PingLiuPing @kevinwilfong @mbasmanova @facebook-github-bot