Skip to content

Support of keyspace field for BATCH message #1778

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Support of keyspace field for BATCH message #1778

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6224a0b
Support of keyspace field for BATCH message
worryg0d Jul 12, 2024
4374c11
added SetKeyspace method to Batch that allows to specify the keyspace…
worryg0d Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions cassandra_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3288,3 +3288,46 @@ func TestQuery_NamedValues(t *testing.T) {
t.Fatal(err)
}
}

func TestBatchKeyspaceField(t *testing.T) {
session := createSession(t)
defer session.Close()

if session.cfg.ProtoVersion < protoVersion5 {
t.Skip("keyspace for BATCH message is not supported in protocol < 5")
}

err := createTable(session, "CREATE TABLE batch_keyspace(id int, value text, PRIMARY KEY (id))")
Copy link
Member

@lukasz-antoniak lukasz-antoniak Aug 27, 2024

Choose a reason for hiding this comment

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

You should create table in different keyspace, and then do b.keyspace = "foo". Query session.Query("SELECT * FROM batch_keyspace") should also override the default keyspace.

Copy link
Member

@lukasz-antoniak lukasz-antoniak Oct 8, 2024

Choose a reason for hiding this comment

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

This has been addressed in #1822.

if err != nil {
t.Fatal(err)
}

ids := []int{1, 2}
texts := []string{"val1", "val2"}

b := session.NewBatch(LoggedBatch)
b.Query("INSERT INTO batch_keyspace(id, value) VALUES (?, ?)", ids[0], texts[0])
Copy link
Member

@lukasz-antoniak lukasz-antoniak Aug 27, 2024

Choose a reason for hiding this comment

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

I think we should also do the test when one of queries also overrides the keyspace.

Copy link
Contributor Author

@worryg0d worryg0d Sep 12, 2024

Choose a reason for hiding this comment

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

As far as I see from the proto 5 spec we can not override the keyspace for the specific query in the Batch, only for the Batch itself.

4.1.7. BATCH

  Allows executing a list of queries (prepared or not) as a batch (note that
  only DML statements are accepted in a batch). The body of the message must
  be:
    <type><n><query_1>...<query_n><consistency><flags>[<serial_consistency>][<timestamp>][<keyspace>][<now_in_seconds>]

Copy link
Member

@lukasz-antoniak lukasz-antoniak Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see that my comment might not be understandable. You can simulate hardcoding keyspace in CQL string itself. But, yeah it is not a very useful test indeed.

b.Query("INSERT INTO batch_keyspace(id, value) VALUES (?, ?)", ids[1], texts[1])
err = session.ExecuteBatch(b)
if err != nil {
t.Fatal(err)
}

var (
id int
text string
)

iter := session.Query("SELECT * FROM batch_keyspace").Iter()
defer iter.Close()

for i := 0; iter.Scan(&id, &text); i++ {
if id != ids[i] {
Copy link
Member

@lukasz-antoniak lukasz-antoniak Aug 27, 2024

Choose a reason for hiding this comment

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

Maybe use require.Equal(t, ...).

t.Fatalf("expected id %v, got %v", ids[i], id)
}

if text != texts[i] {
t.Fatalf("expected text %v, got %v", texts[i], text)
}
}
}
4 changes: 4 additions & 0 deletions conn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1554,6 +1554,10 @@ func (c *Conn) executeBatch(ctx context.Context, batch *Batch) *Iter {
customPayload: batch.CustomPayload,
}

if c.version > protoVersion4 {
req.keyspace = c.currentKeyspace
Copy link
Member

@lukasz-antoniak lukasz-antoniak Aug 27, 2024

Choose a reason for hiding this comment

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

The purpose of introducing keyspace field in the protocol is not to copy default keyspace that we used when establishing connection, but to be able to override keyspace on per-statement basis. You need to specify keyspace from Batch type.

Copy link
Contributor Author

@worryg0d worryg0d Sep 12, 2024

Choose a reason for hiding this comment

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

I totally agree with you. I added the SetKeyspace field for the Batch to allow overriding keyspace for the specific batch.

Copy link
Member

@lukasz-antoniak lukasz-antoniak Oct 8, 2024

Choose a reason for hiding this comment

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

This has been addressed in #1822.

}

stmts := make(map[string]string, len(batch.Entries))

for i := 0; i < n; i++ {
Expand Down
14 changes: 14 additions & 0 deletions frame.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1659,6 +1659,9 @@ type writeBatchFrame struct {

//v4+
customPayload map[string][]byte

//v5+
keyspace string
}

func (w *writeBatchFrame) buildFrame(framer *framer, streamID int) error {
Expand Down Expand Up @@ -1718,6 +1721,13 @@ func (f *framer) writeBatchFrame(streamID int, w *writeBatchFrame, customPayload
flags |= flagDefaultTimestamp
}

if w.keyspace != "" {
if f.proto < protoVersion5 {
panic(fmt.Errorf("the keyspace can only be set with protocol 5 or higher"))
}
flags |= flagWithKeyspace
}

if f.proto > protoVersion4 {
f.writeUint(uint32(flags))
} else {
Expand All @@ -1737,6 +1747,10 @@ func (f *framer) writeBatchFrame(streamID int, w *writeBatchFrame, customPayload
}
f.writeLong(ts)
}

if w.keyspace != "" {
f.writeString(w.keyspace)
}
}

return f.finish()
Expand Down