Skip to content

Add gotData and endOfProcess handlers #1564

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

velom
Copy link

@velom velom commented Jun 2, 2025

Summary

Add two new handlers to track the processing:

  1. Helper WithGotData allows to pass a function will be triggered on every processing data block.
  2. Helper WithEndOfProcess allows to pass a function will be triggered when the whole processing going to be finished.

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG

@CLAassistant
Copy link

CLAassistant commented Jun 2, 2025

CLA assistant check
All committers have signed the CLA.

@@ -32,6 +32,8 @@ type onProcess struct {
progress func(*Progress)
profileInfo func(*ProfileInfo)
profileEvents func([]ProfileEvent)
gotData func()
Copy link

Choose a reason for hiding this comment

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

What's the difference of gotData vs data? Other than the function signature?

Copy link
Author

@velom velom Jun 2, 2025

Choose a reason for hiding this comment

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

data is "internal" handler set by library itself during query start:

var (
errors = make(chan error, 1)
stream = make(chan *proto.Block, bufferSize)
)
go func() {
onProcess.data = func(b *proto.Block) {
stream <- b
}
err := c.process(ctx, onProcess)
if err != nil {
c.debugf("[query] process error: %v", err)
errors <- err
}
close(stream)
close(errors)
release(c, err)
}()

@@ -178,6 +183,9 @@ func (c *connect) handle(ctx context.Context, packet byte, on *onProcess) error
}
if block.Rows() != 0 && on.data != nil {
on.data(block)
if on.gotData != nil {
on.gotData()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
on.gotData()
on.gotData(block)

why doesn't gotData take block?

Copy link
Author

Choose a reason for hiding this comment

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

If you'd like to, I can add it

@SpencerTorres SpencerTorres self-assigned this Jun 2, 2025
@SpencerTorres SpencerTorres self-requested a review June 2, 2025 14:41
Copy link
Member

@SpencerTorres SpencerTorres left a comment

Choose a reason for hiding this comment

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

(I sent a response in Slack but I'll copy here for the record)

I looked at the code here, and I'm hesitant to make these changes since they feel very application-specific (to our internal app). I want to learn more about what the goal of these hooks are.

Specifically these things stand out:

  1. Having hooks for data and gotData is confusing, the names need to be clearer. We could rename data to block and gotData to firstBlock (assuming the goal is to track the first block, or count blocks). Maybe we could rename data to some internal function like _data since it's the only one that is always overridden by the driver.
  2. I saw the code suggestion to add *Block to gotData. I think this would cause some data races if the user had access to these outside of the driver, and so we shouldn't expose them.
  3. endOfProcess can be renamed endOfStream since the server packet is called EndOfStream. This depends on whether your goal is to track the end of processing, or the actual EndOfStream packet.
  4. Do we need endOfProcess to know if a query ended? This should already be detectable through the responding driver.Rows return value.

Overall I want to make sure we aren't adding a solution to something really specific for one application. If we add it we need to test/maintain it, and I am suspicious about these lower level hooks. I feel like there's another way to expose these events, perhaps in the driver.Rows interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants