Implement sql.ValueRow
#3248
Merged
Implement sql.ValueRow
#3248
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
zachmu
reviewed
Oct 3, 2025
jycor
force-pushed
the
james/row2
branch
3 times, most recently
from
f23af30 to
d5d79ec
Compare
jycor
force-pushed
the
james/row2
branch
2 times, most recently
from
6138ab3 to
8079228
Compare
zachmu
reviewed
Oct 24, 2025
There was a problem hiding this comment.
Overall this looks good but see comments.
- Renamings are incomplete
- Need to hide these checks behind a feature flag to avoid doing unnecessary work in the interim.
I think the next phase of this plan is to augment the benchmark runners so that we can see the difference in perf for each of these implementation options when we run benchmarks. Ditto the sql correctness suites, we want to be very sure of this before we enable it by default anywhere.
server/handler.go
Outdated
| r, err = resultForEmptyIter(sqlCtx, rowIter, resultFields) | ||
| } else if analyzer.FlagIsSet(qFlags, sql.QFlagMax1Row) { | ||
| r, err = resultForMax1RowIter(sqlCtx, schema, rowIter, resultFields, buf) | ||
| } else if vr, ok := rowIter.(sql.ValueRowIter); ok && vr.CanSupport(sqlCtx) { |
There was a problem hiding this comment.
We want to put this iteration option behind an environment variable or similar until we know it's ready to start handing production traffic
jycor
force-pushed
the
james/row2
branch
2 times, most recently
from
4b69425 to
646cfd3
Compare
zachmu
approved these changes
Oct 31, 2025
sql/expression/comparison_test.go
Outdated
| require.NoError(t, err) | ||
| return v | ||
| func TestValueComparison(t *testing.T) { | ||
| // TODO |
sql/memory.go
Outdated
| // will return an error and erase all the content of the cache. | ||
| Add2(Row2) error | ||
| AddValueRow(ValueRow) error | ||
| // Get2 gets all rows. |
sql/plan/indexed_table_access.go
Outdated
| } | ||
|
|
||
| func (i *IndexedTableAccess) getLookup2(ctx *sql.Context, row sql.Row2) (sql.IndexLookup, error) { | ||
| func (i *IndexedTableAccess) getLookup2(ctx *sql.Context, row sql.ValueRow) (sql.IndexLookup, error) { |
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.
sql.ValueRowis the reincarnation ofsql.Row2.This is an optimization to the sqlengine that eliminates interface boxing by never placing variables into a
[]interface.sql.ValueRowIteris implemented by:Comparison should only use CompareValue when left and right are both NumericType, so Integers, Floats, Decimal, Bit64, and Year types.