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

refactor: introduce tuple abstraction #954

Closed
wants to merge 2 commits into from

Conversation

tomdcc
Copy link
Contributor

@tomdcc tomdcc commented Sep 21, 2017

This replaces the use of byte[][] in a number of places with a Tuple class that wraps it. This is a necessary change to support doing anything other than blindly reading incoming data into a heap-allocated byte array.

The intended use-case is the flip side of #953. The actual functionality will follow in a subsequent PR if there's support for the refactor and feature.

This replaces the use of byte[][] in a number of places with a Tuple
class that wraps it. This is a necessary change to support doing
anything other than blindly reading incoming data into a heap-
allocated byte array.
@codecov-io
Copy link

codecov-io commented Sep 21, 2017

Codecov Report

Merging #954 into master will increase coverage by 0.89%.
The diff coverage is 75.21%.

@@             Coverage Diff             @@
##             master    #954      +/-   ##
===========================================
+ Coverage      65.9%   66.8%   +0.89%     
- Complexity     3558    3717     +159     
===========================================
  Files           166     167       +1     
  Lines         15223   15994     +771     
  Branches       2458    2712     +254     
===========================================
+ Hits          10033   10685     +652     
- Misses         4023    4096      +73     
- Partials       1167    1213      +46

@vlsi
Copy link
Member

vlsi commented Sep 21, 2017

@tomdcc , have you measured the overhead of new Tuple on the processing of a simple/complex SQL?

@@ -0,0 +1,63 @@
/*
* Copyright (c) 2008, PostgreSQL Global Development Group
Copy link
Member

Choose a reason for hiding this comment

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

date should probably be 2017 for a new file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! Fixed

@davecramer
Copy link
Member

I think the abstraction is worthwhile even if it imposes a performance penalty. The question is at which point is it not worthwhile. So some benchmarking will be necessary

@vlsi
Copy link
Member

vlsi commented Sep 25, 2017

Waiting for the performance tests

@tomdcc
Copy link
Contributor Author

tomdcc commented Sep 26, 2017

Ran the suggested benchmarks:

master vs tuple-class

Numbers are very close, slightly more have master faster than tuple-class, but the numbers are close enough that I think we're seeing noise.

@tomdcc
Copy link
Contributor Author

tomdcc commented Oct 5, 2017

@vlsi ping

@davecramer
Copy link
Member

Is there any reason not to commit this ?

@vlsi
Copy link
Member

vlsi commented Oct 16, 2017

Let's see how it goes.
I'm +-0 re this change.

@paplorinc
Copy link
Contributor

Resolved the conflicts in #1701

@paplorinc
Copy link
Contributor

Merged, please close

@davecramer
Copy link
Member

rebased with PR 1701

@davecramer davecramer closed this Feb 10, 2020
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.

None yet

5 participants