Convert column to float when appending floats to an integer column

[crisptrutski]

Closes #37069

Description

This adds a type detection step during append which checks for columns that would need to be relaxed, and does so if we've configured that transition as OK.

How to verify

Describe the steps to verify that the changes are working as expected.

1. Upload a CSV with an all integer column as a new table.
2. View the table and confirm it has integer type.
3. Append a CSV with the same column containing floats to the same table.
4. View the table and confirm it has a float type, and numbers were not truncated.

Demo

Watch it in action

[crisptrutski]

• Convert column to float when appending floats to an integer column #39493 : 5 dependent PRs (#39706 , #39724 , #39741 and 2 others) 👈
• Use toposort to simplify and fix CSV column type operations #39491
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @crisptrutski and the rest of your teammates on Graphite

[replay-io]

Status	In Progress ↗︎ 51 / 52
Commit	444c2ed
Results	⚠️ 4 Flaky ✅ 2345 Passed

[crisptrutski]

There's a regression, where we're upgrading the type unnecessarily.

Uploading 2.0 into a column of type int should be coerced to 2
expected: [[1 2]]
  actual: [[1 2.0]]
    diff: - [[nil 2]]
          + [[nil 2.0]]

[calherries]

I think we should forget sampling rows here. Uploaded CSVs can only be 50 MB and parsing them should be lightning fast (in the future that is). What we want right now is correctness first, speed second.

[crisptrutski]

Should we remove it from uploads as well then, as a short-term fix for missing the floats in a mostly int column?

[calherries]

yes, although I'm not sure that's the cause of the bug

[crisptrutski]

Interesting, I'll try reproduce it with and without.

[calherries]

This optimization around upload-parsing/get-settings is premature IMO. We just don't care much about the few milliseconds per 1000 rows right now, and it's adding a lot of noise to the code.

By my measure (upload-parsing/get-settings) is really cheap in comparison to everything else we're doing. You could even memoize it to make it really really cheap, but even that is questionably worth the keystrokes.

(def type->check (settings->type->check (upload-parsing/get-settings)))

(time
 (dotimes [_ 100000]
   (value->type type->check "1")))
"Elapsed time: 6440.837959 msecs"

(time
 (dotimes [_ 100000]
   (upload-parsing/get-settings)))
"Elapsed time: 46.310292 msecs"

[crisptrutski]

The purity is the main thing for me.

[calherries]

Oh. Well in that case I just disagree that purity is worth it here, but I do see some benefits so if you feel strongly about it let's keep it

[crisptrutski]

My stance is that you need a really good reason for making things impure. I might get worn down over time by how little existing code is pure.

[crisptrutski]

I like the way you're sanity checking the performance of things we've been discussing recently, it's very easy to reason about this stuff from an comfortable armchair, and be wrong.

I know you're using time just for ballpark numbers, and in this case was fit for purpose, but similarly to how we've started using that nice memory measurement tool, I'm thinking that it would be nice to have a little Criterium¹ harness to use for these kinds of micro benchmarks. The Java configuration we use for our REPL is not that representative of production, tables may turn once escape analysis and inlining kick in for example.

Even when doing microbenchmarks rigorously, it's often misleading for real world applications as soon as allocation or memory look-ups are involved - in a tight loop memory locality is huge. This is obviously not a tight loop, and Clojure is in general debaucherous when it comes to memory layout, but good habits go a long way to preventing headaches when things do matter. Non-local changes can often bring old naive code into a hot path.

My general approach is to avoid any work whenever it is simple to do so, especially when it has other benefits. Pure code is so much easier to test.

Footnotes

1. I see it's been 3 years since it got an update. Maybe there's a new kid in town? Hopefully this is just good-old Clojure stability. ↩

[calherries]

+1 on criterium, we should add it to :dev. I sometimes go to the trouble of using it myself. Point noted on not taking microbenchmarks too seriously, although I do find them handy as ballpark estimates.

My stance is that you need a really good reason for making things impure.

I guess I'm not so much of a purist haha. Pure code makes some code hard to read for little benefit. Sometimes you need a really good reason to make things pure.

[crisptrutski]

All benchmarks are better than guesses, but it'll be really nice to a solid quite of real world benchmarks too.

Code being complex can be a good reason itself, unfortunately its so subjective.

When you just need environmental state, bundling that all up in the first argument is a really easy convention that composes pretty well. You can always partially apply the function to get back the impure version.

This "context first arg" is the approach I typically take for injecting things like database connections, metrics emitters, etc, even in imperative languages like Java. I find it immensely useful to be able to trace where effects can happen - things get so complex otherwise!

[calherries]

Could we use allowed-type-upgrades to limit the type checks? Then relax-type can return only allowed upgrades.

[crisptrutski]

That's a nice idea, but I was thinking to leave this method agnostic of whether it's being used for append. That way we could also use it if we chunk up the original insertion, in which case we'd always allow the upgrade. I guess we could pass in allow-upgrade? as an argument though...

Another subtle issue with this approach occurs if we had any non-leaf ambiguous nodes, see my other comment about wanting to wait until we've converted to column-type. Perhaps we'll never have a use case for non-leaf non-column types, but I'd prefer not to make limiting assumptions in areas where there is bigger low hanging fruit for optimization.

Simplicity is another thing, but in this case having separate lines where we relax and then check if it's allowed is arguably simpler in a Hickey-ian sense than a function that interleaves both.

I'll add this idea to a larger backlog optimization issue, since it can be made to work around the abstract node thing by either putting the abstract nodes into the allowed set (I don't like this), or by implicitly putting all the nodes between each pair into the set as well - perhaps that's a bit too magic, but from the start I liked the idea of just defining the "convex hull" of relaxations we'll allow.

[calherries]

Intuitively, I feel like we should be able to skip this line. During the reduction we should be able to get a list of columns that have new column types by construction, rather than checking whether they are matching afterwards. Maybe there's a reason why you've separated these two things though.

[crisptrutski]

The idea is to avoid projecting to concrete types until we've finished relaxing the type, to avoid a case like turning ::boolean-or-int to ::boolean before we see a later ::int. Only once we turn each into a column-type can we check whether this each "upgrade" is allowed.

This is a bit theoretical, since our only abstract type is a leaf, but I'd rather have a bit more code and computation than make assumptions that could be painful to remove later.

[calherries]

Works for me!

[crisptrutski]

@calherries This should be good-to-go now, just need feedback on where I put the new multi-method.