Query Basic Types in Binary Format

[samuel-massinon]

TODO

• Confirm this is a good way of doing this
• More tests (for all the binary format functions)
• Clean up
• Compare metrics
  • Size of data
  • Speed of parsing
• Document

This PR is focusing on using LibPQ to query integer values.

[samuel-massinon]

Got around to testing this. Here's are some results.

Test Performed	Execute & Download Speed (ns)	Execute & Download Bytes Allocated	Parse Speed (ns)	Parse Bytes Allocated	Bytes Transferred
String 1	7036557543	138313414	222047909	40757869	25556052
String 2	6928509582	138313414	228389655	40757869	25556052
String 3	6962369997	138313414	223927678	40757869	25556052
Binary 1	8206719558	138314518	224727759	40756541	29378777
Binary 2	7957701956	138314518	222612657	40756541	29378777
Binary 3	7957701956	138314518	222612657	40756541	29378777

In this test, using binary was slower to execute and download and transferred more bytes. All the other metrics are pretty indistinguishable.

This makes sense because transferring an integer in string format could save space by having variable length strings, where as transferring it as bytes causes use to transfer all the bytes to represent the number. (an example is the number 1 would be 1 or 2 bytes for a string, but could be 8 bytes as binary data.

[samuel-massinon]

Is this ready to merge into master even though we've only implemented binary for 1 data type?

Should we hold off until we implement more? Should we add a warning if someone tries to use binary data? Add a warning in the docs? Or just have a generic catch all implementation that someone can override?

[arnaudh]

From what I grasped (which is admittedly not much), the PR looks good to me.

Regarding whether to merge and how to signal to users, given that our plan is to only implement a subset of all types for now, we should be fine with merging this, but have clear warnings in place.
In the documentation and docstrings (saying binary format is an experimental feature, listing the types supported), but also the code itself. Ideally we should throw an exception before the query is executed if an unsupported type is requested using binary format, though it's probably not the end of the world if the exception happens after while parsing. As long as we error somewhere.

[samuel-massinon]

In the documentation and docstrings (saying binary format is an experimental feature, listing the types supported), but also the code itself. Ideally we should throw an exception before the query is executed if an unsupported type is requested using binary format, though it's probably not the end of the world if the exception happens after while parsing. As long as we error somewhere.

Currently, if we try parsing something like a decimal, it will error while parsing because instead of parsing something like 1.1 it is parsing \0\x03\0\x02\0\0\0\0\0\x15\x12\x8c\x0e? and erroring.

I could add something like

function Base.parse(::Type{T}, pqv::PQValue{R, BINARY})  where {T, R}
    error("Parsing for Type $T and OID $R in binary has not been implemented")
end

How does that seem?

[arnaudh]

Currently, if we try parsing something like a decimal, it will error while parsing because instead of parsing something like 1.1 it is parsing \0\x03\0\x02\0\0\0\0\0\x15\x12\x8c\x0e? and erroring

Ah, yeah we shouldn't rely on the bytes being parseable or not for different types (I can imagine the same bytes being valid representations of different types). I was assuming that with the existing method signatures we'd get a "no method matching" error if we passed a type that we don't support for binary. If not, then yes it sounds like your proposed solution would achieve that (but better check with @iamed2).

[iamed2]

Adding a not-implemented method is an antipattern; we should rely on errors before dispatch (MethodErrors or other). I think we can accomplish this by restricting a lot of the default/fallback methods to where they're actually applicable. For example, Base.parse(::Type{String}, pqv::PQValue) is invalid; this should be limited to Base.parse(::Type{String}, pqv::PQValue{OID,TEXT}) where OID and for BINARY only with string-like OIDs. You should do a survey of existing methods on PQValue and see if they apply to BINARY PQValues.

As an aside, it might be useful to have:

const PQTextValue{OID} = PQValue{OID,TEXT}
const PQBinaryValue{OID} = PQValue{OID,BINARY}

One thing I also see is the column type defaulting to String. Perhaps we can avoid that for binary results, as it doesn't make much sense as a default there. The default could be removed, or we could default to Vector{UInt8}.

[iamed2]

Is this ready to merge into master even though we've only implemented binary for 1 data type?

Should we hold off until we implement more? Should we add a warning if someone tries to use binary data? Add a warning in the docs? Or just have a generic catch all implementation that someone can override?

I think we should implement, to start:

• int2, int4, int8 (done)
• float4, float8 (should be the same as int4 and int8 but reinterpreted as a Float32 or Float64)
• "char", varchar, bpchar, text
• if easy, numeric

[iamed2]

Don't put where clauses in brackets if they are in long-form function definitions and fit on a single line.

[iamed2]

No dangling assignments

https://github.com/invenia/BlueStyle/#whitespace

[iamed2]

Please apply this in other places as well. I will make an issue for including specifically chained assignments on one line in BlueStyle as well to be official.

[iamed2]

How close is this test section to tests above? Could any of it be put in a loop over (TEXT, BINARY)? Especially around the data parsing, could it loop over the data and decide whether to mark it as broken/unimplemented depending on whether the type is part of an implemented list of types? This would help remove duplication of tests and avoid them getting out of sync.

[iamed2]

Docstrings should be updated for the binary_format kwarg, noting that type support is very limited.

Please make this issue: #223 (comment)

[samuel-massinon]

Here's the issue for numeric types #225

[samuel-massinon]

It seems that most of the coverage misses in the diff are where the formatter added more lines. I don't think any code that I wrote and was being hit before is missed.

[iamed2]

Be sure to squash when (or before) merging!