Merge pull request #565 from vizzuhq/seriesdescriptor-handling-right

Refactor C++ SeriesIndex based on typescript SeriesDescriptor

Author: schaumb

CHANGELOG.md

@@ -2,9 +2,10 @@
 
 ## [Unreleased]
 
-### Fixed 
+### Fixed
 
 - Area charts with data series started with zero: tooltip fixed.
+- Series whose contained ',' and aggregated were not working properly.
 
 ## [0.12.0] - 2024-07-29
 

src/apps/weblib/ts-api/module/cchart.ts

@@ -3,16 +3,13 @@ import { CString, CFunction, CEventPtr } from '../cvizzu.types'
 import * as Anim from '../types/anim.js'
 import * as Config from '../types/config.js'
 import * as Styles from '../types/styles.js'
-import * as Data from '../types/data.js'
 
 import { CManagedObject, CObject, CEnv } from './cenv.js'
 import { CPointerClosure } from './objregistry.js'
 import { CProxy } from './cproxy.js'
 import { CCanvas } from './ccanvas.js'
 import { CAnimation } from './canimctrl.js'
 
-import { isIterable } from '../utils.js'
-
 /** Stored Chart object. */
 export class Snapshot extends CManagedObject {}
 
@@ -136,42 +133,15 @@ export class CChart extends CManagedObject {
 			this._wasm._chart_getList,
 			this._wasm._chart_getValue,
 			this._wasm._chart_setValue,
-			(value: unknown): value is Record<string, unknown> =>
-				isIterable(value) && !this._isSeriesDescriptor(value),
-			(value: unknown): string => {
-				// workaround: we should be able to pass series descriptor as two string
-				if (this._isSeriesDescriptor(value)) {
-					return value.aggregator ? `${value.aggregator}(${value.name})` : value.name
-				} else {
-					return String(value).toString()
-				}
-			},
 			(path: string, value: string): unknown => {
 				// workaround because channel.*.set returns already json instead of scalars
 				if (path.startsWith('channels.') && path.endsWith('.set')) {
-					return JSON.parse(value).map((v: string) => this._toSeriesDescriptor(v))
+					return JSON.parse(value)
 				} else return value
 			}
 		)
 	}
 
-	private _toSeriesDescriptor(value: string): Data.SeriesDescriptor {
-		const pattern = /^(\w+)\((.*?)\)$/
-		const match = value.match(pattern)
-		if (match) {
-			return {
-				name: match[2]!,
-				aggregator: match[1]! as Data.AggregatorType
-			}
-		} else {
-			return { name: value }
-		}
-	}
-
-	private _isSeriesDescriptor(value: unknown): value is Data.SeriesDescriptor {
-		return typeof value === 'object' && value !== null && 'name' in value
-	}
-
 	private _makeStyle(computed: boolean): CStyle {
 		return new CStyle(
 			this.getId,

src/apps/weblib/ts-api/module/cproxy.ts

@@ -1,7 +1,7 @@
 import { CPointer, CString } from '../cvizzu.types'
 
 import { CObject } from './cenv.js'
-import { mirrorObject, iterateObject, isIterable, type ShouldIterate } from '../utils.js'
+import { mirrorObject, iterateObject } from '../utils.js'
 import { Mirrored } from '../tsutils.js'
 import { CPointerClosure } from './objregistry.js'
 
@@ -13,8 +13,6 @@ export class CProxy<T> extends CObject {
 	private _lister: Lister
 	private _getter: Getter
 	private _setter: Setter
-	private _shouldIterate: ShouldIterate
-	private _toString: (value: unknown) => string
 	private _fromString: (path: string, str: string) => unknown
 
 	constructor(
@@ -23,21 +21,17 @@ export class CProxy<T> extends CObject {
 		lister: Lister,
 		getter: Getter,
 		setter: Setter,
-		shouldIterate?: ShouldIterate,
-		toString?: (value: unknown) => string,
 		fromString?: (path: string, str: string) => unknown
 	) {
 		super(getId, cenv)
 		this._lister = lister
 		this._getter = getter
 		this._setter = setter
-		this._shouldIterate = shouldIterate || isIterable
-		this._toString = toString || ((value: unknown): string => String(value).toString())
 		this._fromString = fromString || ((_path: string, str: string): unknown => str)
 	}
 
 	set(value: T): void {
-		iterateObject(value, this.setParam.bind(this), this._shouldIterate)
+		iterateObject(value, this.setParam.bind(this))
 	}
 
 	get(): Mirrored<T> {
@@ -64,7 +58,7 @@ export class CProxy<T> extends CObject {
 
 	setParam(path: string, value: unknown): void {
 		const cpath = this._toCString(path)
-		const cvalue = this._toCString(this._toString(value))
+		const cvalue = this._toCString(String(value).toString())
 		try {
 			this._call(this._setter)(cpath, cvalue)
 		} finally {

src/apps/weblib/ts-api/utils.ts

@@ -27,24 +27,18 @@ export function recursiveCopy<T>(value: T, Ignore?: new (...args: never[]) => un
 }
 
 type Visitor = (path: string, value: unknown) => void
-export type ShouldIterate = (value: unknown) => value is Record<string, unknown>
 
 export function isIterable(value: unknown): value is Record<string, unknown> {
 	return typeof value === 'object' && value !== null
 }
 
-export function iterateObject<T>(
-	obj: T,
-	paramHandler: Visitor,
-	shouldIterate: ShouldIterate = isIterable,
-	path: string = ''
-): void {
+export function iterateObject<T>(obj: T, paramHandler: Visitor, path: string = ''): void {
 	if (obj && obj !== null && typeof obj === 'object') {
 		Object.keys(obj).forEach((key) => {
 			const newPath = path + (path.length === 0 ? '' : '.') + key
 			const value = obj[key as keyof T]
-			if (shouldIterate(value)) {
-				iterateObject(value, paramHandler, shouldIterate, newPath)
+			if (isIterable(value)) {
+				iterateObject(value, paramHandler, newPath)
 			} else {
 				paramHandler(newPath, value)
 			}

src/base/type/uniquelist.h

@@ -141,6 +141,11 @@ template <class T> class UniqueList
 		return true;
 	}
 
+	[[nodiscard]] T pop_back()
+	{
+		return extract(items.find(last->first)).key();
+	}
+
 	[[nodiscard]] iterator<> begin() const noexcept
 	{
 		return {first};

src/chart/options/config.cpp

@@ -211,8 +211,29 @@ void Config::setChannelParam(const std::string &path,
 		if (parts.size() == 3 && value == "null")
 			channel.reset();
 		else {
-			if (std::stoi(parts.at(3)) == 0) channel.reset();
-			channel.addSeries({value, table});
+			if (const std::string_view command =
+			        parts.size() == 4 ? std::string_view{"name"}
+			                          : parts.at(4);
+			    command == "name") {
+				if (std::stoi(parts.at(3)) == 0) channel.reset();
+				if (value.empty()) { channel.measureId.emplace(); }
+				else
+					channel.addSeries({value, table});
+			}
+			else if (command == "aggregator") {
+				if (value != "null") {
+					if (!channel.measureId.has_value())
+						channel.measureId.emplace(
+						    channel.dimensionIds.pop_back());
+
+					channel.measureId->setAggr(value);
+				}
+				else if (channel.measureId)
+					channel.measureId->setAggr(
+					    channel.measureId->getColIndex().empty()
+					        ? "count"
+					        : "sum");
+			}
 		}
 		return;
 	}
@@ -239,10 +260,8 @@ std::string Config::getChannelParam(const std::string &path) const
 	if (property == "set") {
 		std::string res;
 		Conv::JSONArr arr{res};
-		if (auto &&measure = channel.measureId)
-			arr << measure->toString();
-		for (auto &&dim : channel.dimensions())
-			arr << dim.getColIndex();
+		if (auto &&measure = channel.measureId) arr << *measure;
+		for (auto &&dim : channel.dimensions()) arr << dim;
 		return res;
 	}
 

src/dataframe/impl/data_source.h

@@ -29,6 +29,12 @@ enum class error_type {
 	internal_error
 };
 
+struct series_meta_t
+{
+	std::string_view name;
+	series_type type;
+};
+
 [[maybe_unused]] [[noreturn]] void error(error_type err,
     std::string_view arg = {});
 

src/dataframe/impl/dataframe.cpp

@@ -722,8 +722,7 @@ std::string dataframe::get_record_id_by_dims(std::size_t my_record,
 	return get_data_source().get_id(my_record, dimensions);
 }
 
-dataframe::series_meta_t dataframe::get_series_meta(
-    const std::string &id) const
+series_meta_t dataframe::get_series_meta(const std::string &id) const
 {
 	switch (auto &&ser = get_data_source().get_series(id)) {
 		using enum series_type;

src/dataframe/impl/dataframe.h

@@ -117,12 +117,6 @@ class dataframe
 	[[nodiscard]] std::span<const std::string> get_categories(
 	    const std::string_view &dimension) const &;
 
-	struct series_meta_t
-	{
-		std::string_view name;
-		series_type type;
-	};
-
 	[[nodiscard]] series_meta_t get_series_meta(
 	    const std::string &id) const;
 

src/dataframe/old/datatable.cpp

@@ -8,6 +8,7 @@
 #include "base/text/funcstring.h"
 #include "chart/options/options.h"
 #include "dataframe/impl/aggregators.h"
+#include "dataframe/impl/data_source.h"
 #include "dataframe/interface.h"
 
 namespace Vizzu::Data
@@ -53,29 +54,21 @@ const MultiIndex &DataCube::iterator_t::operator*() const
 	return index;
 }
 
+SeriesIndex::SeriesIndex(dataframe::series_meta_t const &meta) :
+    name{meta.name}
+{
+	if (meta.type == dataframe::series_type::measure)
+		aggregator.emplace(dataframe::aggregator_type::sum);
+}
+
 SeriesIndex::SeriesIndex(std::string const &str,
     const DataTable &table) :
-    orig_name(str)
+    SeriesIndex(table.getDf().get_series_meta(str))
+{}
+
+void SeriesIndex::setAggr(const std::string &aggr)
 {
-	constinit static auto names =
-	    Refl::get_names<dataframe::aggregator_type>();
-	if (const Text::FuncString func(str, false);
-	    !func.isEmpty()
-	    && std::find(names.begin(), names.end(), func.getName())
-	           != names.end()) {
-		aggr = Refl::get_enum<dataframe::aggregator_type>(
-		    func.getName());
-		if (!func.getParams().empty())
-			sid = table.getDf()
-			          .get_series_meta(func.getParams().at(0))
-			          .name;
-	}
-	else {
-		auto &&[s, type] = table.getDf().get_series_meta(str);
-		sid = s;
-		if (type == DataTable::Type::measure)
-			aggr = dataframe::aggregator_type::sum;
-	}
+	aggregator = Refl::get_enum<dataframe::aggregator_type>(aggr);
 }
 
 DataCube::iterator_t DataCube::begin() const

src/dataframe/old/types.h

@@ -7,6 +7,11 @@
 
 #include "../interface.h"
 
+namespace Vizzu::dataframe
+{
+struct series_meta_t;
+}
+
 namespace Vizzu::Data
 {
 
@@ -27,38 +32,38 @@ struct RowWrapper
 
 class SeriesIndex
 {
-	std::string orig_name;
-	std::string_view sid;
-	std::optional<dataframe::aggregator_type> aggr;
+	std::string_view name{};
+	std::optional<dataframe::aggregator_type> aggregator;
+
+	explicit SeriesIndex(dataframe::series_meta_t const &meta);
 
 public:
+	SeriesIndex() : aggregator(dataframe::aggregator_type::count) {}
 	SeriesIndex(std::string const &str, const DataTable &table);
 
+	void setAggr(const std::string &aggr);
+
 	[[nodiscard]] const dataframe::aggregator_type &getAggr() const
 	{
-		return *aggr;
+		return *aggregator;
 	}
 
 	[[nodiscard]] const std::string_view &getColIndex() const
 	{
-		return sid;
+		return name;
 	}
 
-	[[nodiscard]] bool operator==(const SeriesIndex &rhs) const
-	{
-		return sid == rhs.sid && aggr == rhs.aggr;
-	}
-
-	[[nodiscard]] bool operator<(const SeriesIndex &rhs) const
-	{
-		return sid < rhs.sid || (sid == rhs.sid && aggr < rhs.aggr);
-	}
+	[[nodiscard]] bool operator==(
+	    const SeriesIndex &rhs) const = default;
+	[[nodiscard]] auto operator<=>(
+	    const SeriesIndex &rhs) const = default;
 
-	[[nodiscard]] bool isDimension() const { return !aggr; }
+	[[nodiscard]] bool isDimension() const { return !aggregator; }
 
-	[[nodiscard]] const std::string &toString() const
+	[[nodiscard]] consteval static auto members()
 	{
-		return orig_name;
+		return std::tuple{&SeriesIndex::name,
+		    &SeriesIndex::aggregator};
 	}
 };