Fix dimension aggregation with NaV value

vizzuhq · Aug 21, 2024 · d581beb · d581beb
1 parent 115668e
commit d581beb
Show file tree

Hide file tree

Showing 18 changed files with 38 additions and 54 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,7 @@
 
 - Area charts with data series started with zero: tooltip fixed.
 - Series whose contained ',' and aggregated were not working properly.
+- Dimension columns with non-existent values aggregation was undefined.
 
 ## [0.12.0] - 2024-07-29
 

diff --git a/src/apps/qutils/canvas.cpp b/src/apps/qutils/canvas.cpp
@@ -6,6 +6,8 @@
 #include <QLinearGradient>
 #include <QScreen>
 
+// NOLINTBEGIN(misc-include-cleaner,readability-avoid-nested-conditional-operator)
+
 QColor toQColor(const Gfx::Color &color)
 {
 	return {color.getRedByte(),
@@ -276,4 +278,6 @@ QFont BaseCanvas::fromGfxFont(const Gfx::Font &newFont, QFont font)
 	                  ? QFont::StyleOblique
 	                  : QFont::StyleNormal);
 	return font;
-}
+}
+
+// NOLINTEND(misc-include-cleaner,readability-avoid-nested-conditional-operator)
diff --git a/src/base/geom/circle.cpp b/src/base/geom/circle.cpp
@@ -4,8 +4,8 @@
 #include <cmath>
 #include <numbers>
 #include <optional>
-#include <stdexcept>
 
+#include "base/geom/point.h"
 #include "base/math/floating.h"
 #include "base/math/tolerance.h"
 

diff --git a/src/base/geom/quadrilateral.cpp b/src/base/geom/quadrilateral.cpp
@@ -1,9 +1,10 @@
 #include "quadrilateral.h"
 
-#include "base/math/tolerance.h"
+#include <algorithm>
+
+#include "base/math/floating.h"
 
 #include "point.h"
-#include "rect.h"
 #include "triangle.h"
 
 namespace Geom

diff --git a/src/base/gfx/pathsampler.cpp b/src/base/gfx/pathsampler.cpp
@@ -1,9 +1,11 @@
 #include "pathsampler.h"
 
+#include <cmath>
 #include <cstddef>
 
 #include "base/geom/point.h"
 #include "base/geom/triangle.h"
+#include "base/math/floating.h"
 
 namespace Gfx
 {

diff --git a/src/chart/rendering/markers/abstractmarker.cpp b/src/chart/rendering/markers/abstractmarker.cpp
@@ -1,5 +1,9 @@
 #include "abstractmarker.h"
 
+#include <array>
+#include <cstddef>
+#include <utility>
+
 #include "base/anim/interpolated.h"
 #include "base/geom/line.h"
 #include "chart/generator/marker.h"

diff --git a/src/chart/rendering/renderedchart.cpp b/src/chart/rendering/renderedchart.cpp
@@ -1,13 +1,13 @@
 #include "renderedchart.h"
 
 #include <algorithm>
+#include <limits>
 #include <ranges>
 #include <variant>
 
 #include "base/geom/point.h"
 #include "base/geom/transformedrect.h"
 #include "base/math/floating.h"
-#include "base/math/fuzzybool.h"
 #include "base/util/eventdispatcher.h"
 #include "chart/generator/marker.h" // NOLINT(misc-include-cleaner)
 #include "chart/options/shapetype.h"

diff --git a/src/dataframe/impl/aggregators.cpp b/src/dataframe/impl/aggregators.cpp
@@ -114,15 +114,6 @@ get_aggregators() noexcept
 		                *std::get_if<const std::string *>(&cell))
 			        set.insert(v);
 		        return static_cast<double>(set.size());
-	        }},
-	    {aggrs[static_cast<std::size_t>(aggregator_type::exists)],
-	        empty_double,
-	        [](custom_aggregator::id_type &id,
-	            cell_reference const &cell) -> double
-	        {
-		        auto &b = *std::get_if<double>(&id);
-		        if (is_valid(cell)) b = 1.0;
-		        return b;
 	        }}}}};
 }
 

diff --git a/src/dataframe/impl/data_source.cpp b/src/dataframe/impl/data_source.cpp
@@ -238,14 +238,11 @@ cell_reference data_source::get_data(std::size_t record_id,
 		using enum series_type;
 	case dimension: {
 		const auto &dims = unsafe_get<dimension>(series).second;
-		if (record_id >= dims.values.size()) return nullptr;
 		return dims.get(record_id);
 	}
 	case measure: {
 		const auto &meas = unsafe_get<measure>(series).second;
-		if (record_id >= meas.values.size()) return nan;
-
-		return meas.values[record_id];
+		return meas.get(record_id);
 	}
 	case unknown:
 	default: error(error_type::series_not_found, column);
@@ -627,9 +624,9 @@ void data_source::dimension_t::add_more_data(
 const std::string *data_source::dimension_t::get(
     std::size_t index) const
 {
-	return values[index] == nav
-	         ? nullptr
-	         : std::addressof(categories[values[index]]);
+	return index < values.size() && values[index] != nav
+	         ? std::addressof(categories[values[index]])
+	         : nullptr;
 }
 
 void data_source::dimension_t::set(std::size_t index,

diff --git a/src/dataframe/impl/dataframe.cpp b/src/dataframe/impl/dataframe.cpp
@@ -94,9 +94,7 @@ dataframe::dataframe(std::shared_ptr<const data_source> other,
 void valid_unexistent_aggregator(const std::string_view &series,
     const dataframe::any_aggregator_type &agg)
 {
-	if (!agg
-	    || (*agg != aggregator_type::count
-	        && *agg != aggregator_type::exists))
+	if (!agg || *agg != aggregator_type::count)
 		error(error_type::series_not_found, series);
 }
 

diff --git a/src/dataframe/interface.h b/src/dataframe/interface.h
@@ -27,8 +27,7 @@ enum class aggregator_type : std::uint8_t {
 	max,
 	mean,
 	count,
-	distinct,
-	exists
+	distinct
 };
 
 enum class sort_type : std::uint8_t {
@@ -63,11 +62,6 @@ struct custom_aggregator
 		return name <=> oth.name;
 	}
 
-	auto operator!=(const custom_aggregator &oth) const
-	{
-		return name != oth.name;
-	}
-
 	auto operator==(const custom_aggregator &oth) const
 	{
 		return name == oth.name;

diff --git a/src/dataframe/old/datatable.cpp b/src/dataframe/old/datatable.cpp
@@ -16,10 +16,8 @@
 #include "base/conv/auto_json.h"
 #include "base/conv/numtostr.h"
 #include "base/refl/auto_enum.h"
-#include "base/text/funcstring.h"
 #include "chart/options/options.h"
 #include "chart/options/shapetype.h"
-#include "dataframe/impl/aggregators.h"
 #include "dataframe/impl/data_source.h"
 #include "dataframe/interface.h"
 

diff --git a/test/qtest/chart.cpp b/test/qtest/chart.cpp
@@ -146,10 +146,9 @@ void TestChart::run()
 			IO::log() << "step 1b";
 			auto &options = chart.getChart().getOptions();
 			auto &styles = chart.getChart().getStyles();
-			options.dataFilter =
-			    Vizzu::Data::Filter{std::shared_ptr<bool(
-			        const Vizzu::Data::RowWrapper *)>{
-			        std::shared_ptr<void>{},
+			options.dataFilter = Vizzu::Data::Filter{
+			    std::unique_ptr<bool(const Vizzu::Data::RowWrapper *),
+			        void (*)(bool(const Vizzu::Data::RowWrapper *))>{
 			        +[](const Vizzu::Data::RowWrapper *row) -> bool
 			        {
 				        return *std::get<const std::string *>(
@@ -158,6 +157,9 @@ void TestChart::run()
 				            || *std::get<const std::string *>(
 				                   row->get_value("Cat2"))
 				                   == std::string_view{"b"};
+			        },
+			        +[](bool(const Vizzu::Data::RowWrapper *))
+			        {
 			        }}};
 			options.title = "VIZZU Chart - Phase 1b";
 			styles.legend.marker.type =

diff --git a/test/unit/base/refl/auto_enum.cpp b/test/unit/base/refl/auto_enum.cpp
@@ -38,7 +38,7 @@ template <typename T> std::string toString(T v)
 {
 	return Refl::enum_name<std::string>(v);
 }
-template <typename T> T parse(std::string s)
+template <typename T> T parse(const std::string &s)
 {
 	return Refl::get_enum<T>(s);
 }
@@ -120,7 +120,8 @@ const static auto tests =
             {
 	            throws<std::logic_error>() << []
 	            {
-		            return Refl::enum_name(Foo::fobar{2});
+		            return Refl::enum_name(
+		                std::bit_cast<Foo::fobar>(2));
 	            };
             })
 

diff --git a/test/unit/chart/events.cpp b/test/unit/chart/events.cpp
@@ -55,6 +55,7 @@ struct MyCanvas final : Gfx::ICanvas, Vizzu::Draw::Painter
 	void frameBegin() final {}
 	void frameEnd() final {}
 	void *getPainter() final { return static_cast<Painter *>(this); }
+	// cppcheck-suppress duplInheritedMember
 	ICanvas &getCanvas() final { return *this; }
 };
 
@@ -333,6 +334,7 @@ std::multimap<std::string, event_as, std::less<>> get_events(
 				        "draw-" + std::string{s.back()});
 			    }
 
+			    // cppcheck-suppress uselessCallsConstructor
 			    if (s.back() == "base") s = {s.begin(), s.end() - 1};
 
 			    std::string name;

diff --git a/test/unit/dataframe/interface_test.cpp b/test/unit/dataframe/interface_test.cpp
@@ -57,6 +57,7 @@ struct if_setup
 
 			skip->*df->get_dimensions() == dims;
 			skip->*df->get_measures() == meas;
+			// cppcheck-suppress ignoredReturnValue
 			skip->*df->get_record_count() == data.size();
 
 			for (auto r = 0u; r < data.size(); ++r) {
@@ -77,6 +78,7 @@ struct if_setup
 					    std::get<double>(df->get_data(r, meas[m]));
 					auto &&nData = data[r][m + ds];
 					if (nData && std::isnan(gdata)) continue;
+					// cppcheck-suppress ignoredReturnValue
 					skip->*gdata == std::stod(data[r][m + ds]);
 				}
 			}
@@ -484,13 +486,11 @@ const static auto tests =
 	auto &&pure1c = df->set_aggregate({}, count);
 	auto &&d1c = df->set_aggregate("d1", count);
 	auto &&d1d = df->set_aggregate("d1", distinct);
-	auto &&d1e = df->set_aggregate("d1", exists);
 	auto &&m1s = df->set_aggregate("m1", sum);
 	auto &&m1mi = df->set_aggregate("m1", min);
 	auto &&m1ma = df->set_aggregate("m1", max);
 	auto &&m1me = df->set_aggregate("m1", mean);
 	auto &&m1c = df->set_aggregate("m1", count);
-	auto &&m1e = df->set_aggregate("m1", exists);
 
 	/*
 auto &&m1t = df->set_aggregate("m1",
@@ -524,8 +524,6 @@ auto &&m1t = df->set_aggregate("m1",
 	        d1c,
 	        m1c,
 	        d1d,
-	        d1e,
-	        m1e,
 	        m1s,
 	        m1ma,
 	        m1me,
@@ -539,8 +537,6 @@ auto &&m1t = df->set_aggregate("m1",
 	check->*df->get_data(std::size_t{0}, d1c) == 5.0;
 	check->*df->get_data(std::size_t{0}, m1c) == 5.0;
 	check->*df->get_data(std::size_t{0}, d1d) == 1.0;
-	check->*df->get_data(std::size_t{0}, d1e) == 1.0;
-	check->*df->get_data(std::size_t{0}, m1e) == 1.0;
 	check->*df->get_data(std::size_t{0}, m1ma) == 88.0;
 	check->*df->get_data(std::size_t{0}, m1me) == 21.85;
 	check->*df->get_data(std::size_t{0}, m1mi) == 2.0;
@@ -551,8 +547,6 @@ auto &&m1t = df->set_aggregate("m1",
 	check->*df->get_data(std::size_t{1}, d1c) == 4.0;
 	check->*df->get_data(std::size_t{1}, m1c) == 3.0;
 	check->*df->get_data(std::size_t{1}, d1d) == 1.0;
-	check->*df->get_data(std::size_t{1}, d1e) == 1.0;
-	check->*df->get_data(std::size_t{1}, m1e) == 1.0;
 	check->*df->get_data(std::size_t{1}, m1ma) == 7.25;
 	check->*df->get_data(std::size_t{1}, m1me) == 5.0;
 	check->*df->get_data(std::size_t{1}, m1mi) == 3.5;
@@ -563,8 +557,6 @@ auto &&m1t = df->set_aggregate("m1",
 	check->*df->get_data(std::size_t{2}, d1c) == 1.0;
 	check->*df->get_data(std::size_t{2}, m1c) == 0.0;
 	check->*df->get_data(std::size_t{2}, d1d) == 1.0;
-	check->*df->get_data(std::size_t{2}, d1e) == 1.0;
-	check->*df->get_data(std::size_t{2}, m1e) == 0.0;
 	check
 	            ->*std::isnan(std::get<double>(
 	                df->get_data(std::size_t{2}, m1ma)))
@@ -585,8 +577,6 @@ auto &&m1t = df->set_aggregate("m1",
 	check->*df->get_data(std::size_t{3}, d1c) == 0.0;
 	check->*df->get_data(std::size_t{3}, m1c) == 1.0;
 	check->*df->get_data(std::size_t{3}, d1d) == 0.0;
-	check->*df->get_data(std::size_t{3}, d1e) == 0.0;
-	check->*df->get_data(std::size_t{3}, m1e) == 1.0;
 }
 
     | "aggregate multiple dim" |

diff --git a/test/unit/util/collection.h b/test/unit/util/collection.h
@@ -101,7 +101,7 @@ class collection : public case_registry
 		return stats;
 	}
 
-	void print_summary(statistics stats)
+	void print_summary(const statistics &stats)
 	{
 		std::cout << "\n"
 		          << "all tests:    " << cases.size() << "\n"

diff --git a/test/unit/util/condition.h b/test/unit/util/condition.h
@@ -20,9 +20,8 @@ template <typename T> struct decomposer
 
 	template <class U> void operator==(const U &ref) const
 	{
-		if constexpr (requires { ref == value; }) {
+		if constexpr (requires { ref == value; })
 			return evaluate(value == ref, "==", ref);
-		}
 		else if constexpr (std::ranges::range<T>) {
 			auto &&[lhs, rhs] = std::ranges::mismatch(value, ref);
 			return evaluate(lhs == std::end(value)