Row getVector* methods are now templates #122

ilijapuaca · 2020-05-01T11:03:39Z

This seemed like a pitfall for anyone starting fresh -- all the data loaded through arrow loader would end up using doubles, while in our accessors we're always requesting floats. Current implementation requires ugly casting, or even worse returning the implicit default value without it.

Not sure whether this is the best implementation out there, but it gets rid of ~250 lines of boilerplate code that was in there before.

ibgreen · 2020-05-01T12:49:56Z

cpp/examples/deck.gl/flight-paths.cc

-    // NOTE: Decimal array data is being loaded as double
-    auto startPositions = row.getDoubleVector3("start");
-    return mathgl::Vector3<float>{startPositions};
+    return row.getVector3<float>("start");


An alternative is row.get<Vector3<float>>()

You could do this with partial template specializations.

If I understand correctly, the method signature would end up being auto get(const std::string& columnName, const T& defaultValue = {}) const -> T?

That doesn't sound like a good idea to me as whoever is calling this might expect us to support any type out there, while in reality it would be only vectors. We could throw an error for when specialization isn't implemented for that type, but it just seems unnecessary and it does not seem to add too much value

ibgreen · 2020-05-01T12:50:08Z

cpp/modules/deck.gl/core/src/arrow/row.cc

@@ -41,7 +41,7 @@ auto Row::getInt(const std::string& columnName, int defaultValue) const -> int {

  auto chunk = this->_getChunk(columnName);
  auto doubleValue = this->_getDouble(chunk);
-  if (doubleValue.has_value()) {
+  if (doubleValue) {


This seems like a JavaScript-ism to me. Maybe rename to doubleOption? The value part of the name combined with using auto makes me think doubleValue actually has type `double., for which this if statement doesn't really make sense.

I also agree that using has_value is more verbose and improves the readability, especially when using auto. @ibgreen seems to prefer leaving that out, so I was trying to be consistent.

Updated auto to explicit type as a middle ground

I can see both view points. pointers in C++ are routinely compared as boolean values.

if (pointer) vs if (pointer === nullptr)

And C++ has the conversion to boolean built-in.

std::optional is clearly designed to mimic the pointer idiom. It support similar style if checking, and can even be "dereferenced" using the * operator (instead of get_value)

So - we may want to pick a style (e.g. pointer like vs explicit has/get_value calls) since we are increasingly making use of std::optional.

ibgreen · 2020-05-01T12:54:47Z

cpp/modules/deck.gl/core/src/arrow/row.cc

@@ -104,171 +104,6 @@ auto Row::getString(const std::string& columnName, const std::string& defaultVal
  return defaultValue;
 }

-auto Row::getFloatVector2(const std::string& columnName, const Vector2<float>& defaultValue) const -> Vector2<float> {


One issue with templates is that all the code goes into header files and needs to be compiled (or at least instantiated) again and again for each type, which slows down compilation and bloats the executable. So it can sometimes be good to have a smaller template that does type casting and such, and have bigger non-templated helper methods.

That's exactly what I was trying to do. I don't see a way to make _getVectorData smaller without breaking it down into individual methods that would each return a vector of data. That would be additional overhead on top of what was just added, where we'd be filling 2 vectors and casting them in order to form the final data.

Open to suggestions if you have an improvement in mind, I may be missing something

Sorry I comment linearly as I read through the PR. I was pleasantly surprised at how compact the code was when I got a bit further, but forgot to modify this comment. All good.

ibgreen · 2020-05-01T12:56:10Z

cpp/modules/deck.gl/core/src/arrow/row.cc


-  return Vector4<float>{first, second, third, fourth};
-}
+      return std::make_shared<ListArrayMetadata>(offset, length, listArray->values());


If we are not sharing it, don't return a shared pointer. Just return the struct by value. It is faster and cleaner.

I need a way to check whether it's valid, so it was either this, an optional, or a raw pointer. I guess an optional might make more sense?

Yes - you could add e.g. a valid flag to the struct, or use an optional.

ibgreen · 2020-05-01T12:57:58Z

cpp/modules/deck.gl/core/src/arrow/row.h

-      -> mathgl::Vector4<float>;
-  auto getDoubleVector4(const std::string& columnName, const mathgl::Vector4<double>& defaultValue = {}) const
-      -> mathgl::Vector4<double>;
+  template <typename T>


An alternative is row.get<Vector3<float>>()

You could do this with partial template specializations.

I.e. you would still have all these methods, but they would be specializations rather than different methods.

ibgreen · 2020-05-01T13:02:39Z

cpp/modules/deck.gl/core/src/arrow/row.h

+    auto metadata = this->_getListArrayMetadata(this->_getChunk(columnName), this->_chunkRowIndex);
+
+    auto data = std::make_shared<std::vector<T>>();
+    if (auto doubleArray = std::dynamic_pointer_cast<arrow::DoubleArray>(metadata->values)) {


Dynamic casts are somewhat expensive as they have to inspect RTTI data. It is rarely an issue, only if done very frequently inside loops - but unfortunately that is the case with all Row methods.

I would strongly prefer that we switched on the arrow type constant here instead.

We should set up a simple benchmark so we can test the row method performance (in particular we need to cache the string value lookups).

ilijapuaca · 2020-05-02T04:28:36Z

Landing this as some of the stuff I'm working on calls out to getVector APIs. Will revisit any additional comments in a followup

ibgreen · 2020-05-02T17:19:30Z

cpp/modules/deck.gl/core/src/arrow/row.h

+    auto metadata = optionalMetadata.value();
+    std::vector<T> data;
+    switch (metadata.values->type_id()) {
+      case arrow::Type::type::DOUBLE: {


ibgreen · 2020-05-02T17:20:30Z

cpp/modules/deck.gl/core/src/arrow/row.h

+      -> mathgl::Vector3<T> {
+    auto optionalData = this->_getVectorData<T>(columnName);
+    if (!optionalData) {
+      probegl::WarningLog() << "Unsupported vector data type, returning default value";


Be careful with these warnings, these could cause massive cascades if processing a million row table.

The probe.gl logging system in JS has warning deduplication as a built-in feature. I.e. each warning message string is cached and only printed once.

I could put that in place if it's not too much work, not sure how you went about doing that in JS. For now, I think there should be something in place that notifies the user of the API that something has gone wrong during the conversion though, as ugly as this may be

Just a set recording which strings have already been logged. Could also be a map<string, timestamp> and only log if say 1 second has passed.

I believe the log function has an option.once. This is set by default on log.warn and log.error (the api is slightly different).

Opened up a new issue #129

ibgreen · 2020-05-02T17:23:57Z

cpp/modules/deck.gl/core/src/arrow/row.cc

@@ -41,7 +41,7 @@ auto Row::getInt(const std::string& columnName, int defaultValue) const -> int {

  auto chunk = this->_getChunk(columnName);
  auto doubleValue = this->_getDouble(chunk);
-  if (doubleValue.has_value()) {
+  if (doubleValue) {


I can see both view points. pointers in C++ are routinely compared as boolean values.

if (pointer) vs if (pointer === nullptr)

And C++ has the conversion to boolean built-in.

std::optional is clearly designed to mimic the pointer idiom. It support similar style if checking, and can even be "dereferenced" using the * operator (instead of get_value)

So - we may want to pick a style (e.g. pointer like vs explicit has/get_value calls) since we are increasingly making use of std::optional.

Row getVector* methods are now templates

9666524

ilijapuaca requested a review from ibgreen May 1, 2020 11:03

ibgreen approved these changes May 1, 2020

View reviewed changes

#122 comments addressed

513e3fb

ilijapuaca merged commit 3287fc2 into master May 2, 2020

ilijapuaca deleted the feature/arrow-row-casting branch May 2, 2020 04:28

ibgreen reviewed May 2, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Row getVector* methods are now templates #122

Row getVector* methods are now templates #122

ilijapuaca commented May 1, 2020

ibgreen May 1, 2020

ilijapuaca May 2, 2020

ibgreen May 1, 2020

isaacbrodsky May 1, 2020

ilijapuaca May 2, 2020

ibgreen May 2, 2020

ibgreen May 1, 2020

ilijapuaca May 2, 2020

ibgreen May 2, 2020

ibgreen May 1, 2020

ilijapuaca May 2, 2020

ibgreen May 2, 2020

ibgreen May 1, 2020

ibgreen May 1, 2020

ilijapuaca commented May 2, 2020

ibgreen May 2, 2020

ibgreen May 2, 2020

ilijapuaca May 4, 2020

ibgreen May 4, 2020

ilijapuaca May 5, 2020

ibgreen May 2, 2020

Row getVector* methods are now templates #122

Row getVector* methods are now templates #122

Conversation

ilijapuaca commented May 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilijapuaca commented May 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment