Add data type/schema field/schema #31

lidavidm · 2025-01-16T01:17:17Z

Implement the API for data types and schemas.

lidavidm · 2025-01-16T01:18:46Z

Note: we could simplify the data type definition greatly if we adopt something like Arrow Java/cuDF. Type would be a POD containing type ID and fields for all parameterized types (decimal scale, fixed length, etc.). Field would contain a Type and a (possibly empty/invalid) list of nested fields (to handle things like Struct). This loses type safety but removes a layer of indirection/allocations.

lidavidm · 2025-01-16T01:19:43Z

For here I've chosen something somewhat resembling Arrow C++. However I think Schema and Field (unlike Arrow) should not always be wrapped in smart pointers. Type is still in a smart pointer for type erasure.

src/iceberg/type_fwd.h

src/iceberg/schema.h

Fokko

Looking good, left a few comments

src/iceberg/schema.h

src/iceberg/type.h

src/iceberg/type_fwd.h

src/iceberg/type.h

src/iceberg/type_fwd.h

src/iceberg/type.h

src/iceberg/schema.h

lidavidm · 2025-01-17T07:25:00Z

I added Primitive/NestedType; I'll start filling out definitions and the rest of the types next, along with adding the visitors to iceberg-arrow.

gaborkaszab

Great to see progress on this area, @lidavidm ! I went through for my own education and left some comments or questions. Let me know what you think!

gaborkaszab · 2025-01-17T09:01:28Z

src/iceberg/schema.h

+
+#pragma once
+
+/// \file iceberg/schema.h


I'm not familiar with this style of commenting. Could you please explain me why including the file name into a comment needed here?

I'm not done with it yet, this is for Doxygen which will generate a page to document the header.

gaborkaszab · 2025-01-17T09:03:09Z

src/iceberg/schema.h

+  ///
+  /// Schemas are identified by a unique ID for the purposes of schema
+  /// evolution.
+  [[nodiscard]] int32_t schema_id() const;


The functions within this class may not follow the same naming convention: See schema_id vs ToString, Equals
I found the same mismatch below too, so I'm wondering if it's me missing something here.

Update: I've just checked the style guide, and I guess this is fine: "accessors and mutators may be named like variables"
https://google.github.io/styleguide/cppguide.html#Function_Names

Google style has trivial accessors using snake_case.

gaborkaszab · 2025-01-17T09:04:40Z

src/iceberg/schema.h

+  [[nodiscard]] std::string ToString() const;
+
+  /// \brief Compare two schemas for equality.
+  [[nodiscard]] bool Equals(const Schema& other) const;


If we have operators for equality, can't we make the Equals() function private? I don't see the reason to expose both.

It may be useful to add overloads later (e.g. if we want to ignore metadata or IDs)

Then we can extend this later. I believe in adding what is required ATM, and if there is a need later, we can still extend.
Also, we are defining a public interface here, should keep it as simple as possible. Exposing the Equals method seems a redundancy for me ATM.

src/iceberg/type.h

gaborkaszab · 2025-01-17T09:30:37Z

src/iceberg/type.h

+namespace iceberg {
+
+/// \brief A type combined with a name.
+class ICEBERG_EXPORT Field {


I think similarly to Schema, I'd put Field into a separate file. Now this is in type.h but Field isn't really a type.

There's a bit of a circular dependency between Field (which wants Type) and StructType (which wants Field). Unless (again) you see the alternate design in the first two comments of this PR which puts child Fields always on the Field (this means that Field has a potentially redundant...field, but avoids nesting of types - I personally slightly prefer this but it's conceptually dependent on whether you model the type itself as nested or the field as the nested object)

Sure, will take a look a bit later.
Generally asking, can't we solve the circular dependency with forward declarations and using pointers/references?

We can if we continue to type-erase Type, but not if we want it to not be behind a smart pointer.

Putting them behind smart pointers makes sense. See my other comment. And the this could go into a separate file.
Meanwhile I'm sketching some code for the PartitionSpec and PartitionField and I'm wondering if this one here is intuitive to be called Field, or could be better called SchemaField (considering there is going to be a PartitionField too).

I'll rename

Do you suppose PartitionField to inherit SchemaField or it is totally a separate thing? @gaborkaszab

gaborkaszab · 2025-01-17T11:01:55Z

src/iceberg/schema.h

+  [[nodiscard]] std::string ToString() const;
+
+  /// \brief Compare two schemas for equality.
+  [[nodiscard]] bool Equals(const Schema& other) const;


Then we can extend this later. I believe in adding what is required ATM, and if there is a need later, we can still extend.
Also, we are defining a public interface here, should keep it as simple as possible. Exposing the Equals method seems a redundancy for me ATM.

gaborkaszab · 2025-01-17T11:03:56Z

src/iceberg/schema.h

+/// evolution.
+class ICEBERG_EXPORT Schema : public StructType {
+ public:
+  Schema(int32_t schema_id, std::vector<Field> fields);


One thing I saw in the Java implementation, is that there is a name to field ID mapping there, and there are accessors getting Field(s) by ID or by name. I think that would be useful here too.

gaborkaszab · 2025-01-17T11:19:44Z

src/iceberg/schema.h

+/// A schema is a list of typed columns, along with a unique integer ID.  A
+/// Table may have different schemas over its lifetime due to schema
+/// evolution.
+class ICEBERG_EXPORT Schema : public StructType {


: public StructType
I don't think that Schema should derive from StructType. I get that it will hold a list of Fields such as StructType, but still, Schema in general is not a Type, would be pretty unnatural to derive from it.

(Note that various type systems do actually just represent a schema as a struct type, including nanoarrow/Arrow C Data Interface. At the very least they are isomorphic and you can see methods that convert between them often. That said I don't feel strongly about this.)

I'm also inclined to inherit Schema from StructType. In practice this inheritance avoid an unnecessary conversion from Schema to StructType to implement type visitors.

Just curious: Could visitors work with Schema if we had a Schema::asStructType or such? Doesn't seem a big deal to do that conversion for me and then the class hierarchy would be more intuitive in my opinion. Or I miss something :)

That would also work, it would just add the conversion, and then duplicating all the methods. No big deal either way to me at least.

(I suppose the OOP approach is to add a StructLike interface that both Struct and Schema implement.)

gaborkaszab · 2025-01-17T11:21:28Z

src/iceberg/type.h

+namespace iceberg {
+
+/// \brief A type combined with a name.
+class ICEBERG_EXPORT Field {


Sure, will take a look a bit later.
Generally asking, can't we solve the circular dependency with forward declarations and using pointers/references?

lidavidm · 2025-01-20T02:35:09Z

Just to make sure @gaborkaszab @wgtmac: are we ok with the Arrow-style type representation here? (Types are represented by a class hierarchy, erased behind smart pointers; nested types store the fields in the type object, not the field object)

The alternatives are:

cuDF style: type objects are minimal, just a type ID, and there is no hierarchy, just the base DataType class which is not type-erased and therefore there is no need for a smart pointer. As a trade-off, nested fields have to be extracted from the field and the base DataType has to have fields for all possible parameterized types
arrow-java style: there is still a hierarchy of type objects, but child fields are stored on the Field, not the DataType. Avoids a conceptual dependency cycle between DataType and Field
variant style: like arrow-java, but instead of type erasure and a type hierarchy (or conversely, like cuDF but avoids redundant fields), we have a std::variant with all possibilities. Avoids boxing but the object is larger (in e.g. a vector)

lidavidm · 2025-01-20T02:35:43Z

Arrow-style does let you do a bunch of compile-time metaprogramming (e.g. see arrow::TypeTraits)

mapleFU · 2025-01-20T08:26:45Z

src/iceberg/type.h

+  /// \brief Is this a primitive type (may not have child fields)?
+  [[nodiscard]] virtual bool is_primitive() const = 0;
+
+  /// \brief Is this a nested type (may have child fields)?


What raise my interests is that:

Does iceberg has a "void" ( or null ) bottom type?

Does iceberg allowed to define an empty structure?

There is unknown.

I don't think the spec says anything either way about an empty struct. I didn't check any implementations. If allowed, you could think of it as a void type.

I quickly checked the Java code, and apparently there is nothing preventing us from creating a StructType with zero fields.

The UnknownType will be part of the V3 specification.

An empty struct is pretty common (for example, an unpartitioned table has an empty struct as a partition).

wgtmac · 2025-01-20T14:07:35Z

I vote +1 for the Arrow-style. I haven't checked the cuDF style in detail but it looks more like the C-style. The extra benefit of Arrow-style is that it might be simpler to implement the Iceberg <=> Arrow schema conversion.

mapleFU · 2025-01-20T14:46:11Z

LGTM

gaborkaszab · 2025-01-20T16:35:47Z

Just to make sure @gaborkaszab @wgtmac: are we ok with the Arrow-style type representation here? (Types are represented by a class hierarchy, erased behind smart pointers; nested types store the fields in the type object, not the field object)

If I get it right, Arrow-style mes sense for me too.

lidavidm · 2025-01-21T05:36:08Z

I've implemented all the actual method bodies. I've also added a trait for ToString/std::format formatting (might as well use the new C++ features if we're requiring C++20). Although it appears clang is not happy about what I did.

I've elected to make getters fallible (e.g. GetFieldById) via optional<reference_wrapper<T>> which looks a bit messy but should be safer than returning T const& and throwing, or returning T const* and admitting nullptr.

lidavidm · 2025-01-22T06:21:56Z

Ok, hopefully that fixes macOS. It appears C++20 support on the version of clang shipped by macOS is still flaky:

https://godbolt.org/z/W9rTexc13

Maybe we want to avoid C++20 (sadly) or maybe we need to encourage people to get a newer clang on macOS. (It appears to be clang 15, whereas the latest is clang 19.) Or maybe it's just an issue with the version the CI environment has.

lidavidm · 2025-01-22T06:27:29Z

I'll add some unit tests as long as the API looks good to everyone.

wgtmac

The interface looks good to me. Thanks!

wgtmac · 2025-01-22T08:04:49Z

src/iceberg/schema_field.h

+                                  std::shared_ptr<Type> type);
+
+  /// \brief Get the field ID.
+  [[nodiscard]] int32_t field_id() const;


Is it an overkill to add [[nodiscard]] to these accessors?

I don't think it hurts

Right, it is just personal flavor. I have commented at #32 (comment)

src/iceberg/type.h

lidavidm · 2025-01-23T02:54:43Z

I added a bunch of unit tests + fixed some copy-paste errors I discovered in the process.

raulcd · 2025-01-24T12:11:34Z

src/iceberg/schema_field.cc

+    : field_id_(field_id),
+      name_(std::move(name)),
+      type_(std::move(type)),
+      optional_(optional) {}


I am wondering why python moved to required instead of optional here:
https://github.com/apache/iceberg-python/blob/main/pyiceberg/types.py#L308
The change from optional to required seemed intentional here, @Fokko any thoughts?
BTW it seems Java went for optional too.

It is because the spec serializes it as required: https://iceberg.apache.org/spec/#schemas This way it is more straightforward to serialize it using the library that we use (Pydantic). See the original issue: apache/iceberg#4985. In PyIceberg we have a property called optional as well, so you can choose.

mapleFU

Great work!

src/iceberg/type.cc

mapleFU · 2025-01-24T15:41:03Z

src/iceberg/type.cc

+                      field.field_id(), it->second, index));
+    }
+
+    index++;


nit: ++index;

src/iceberg/type.cc

zhjwpku · 2025-01-25T13:10:44Z

src/iceberg/type_fwd.h

+class Schema;
+class SchemaField;
+class StringType;
+class StructType;


typo: redundant

zhjwpku · 2025-01-25T13:28:53Z

src/iceberg/schema.h

+
+  /// \brief Get the schema ID.
+  ///
+  /// Schemas are identified by a unique ID for the purposes of schema


nit: should this be A Schema is identified by a unique ID or Schemas are identified by unique IDs?

Changed to "A schema..."

zhjwpku · 2025-01-25T14:08:42Z

src/iceberg/type.cc

+  return other.type_id() == TypeId::kBoolean;
+}
+
+TypeId Int32Type::type_id() const { return TypeId::kInt32; }


Should we use Int/Long/Float/Double instead? I guess this arrow style, but I think we should be consistent with spec? I checked iceberg and pyiceberg, they do the spec way.

zhjwpku · 2025-01-25T14:10:47Z

src/iceberg/type.cc

+  return precision_ == decimal.precision_ && scale_ == decimal.scale_;
+}
+
+TypeId TimeType::type_id() const { return TypeId::kTime; }


nit: place TimeType after DateType

Reordered headers/impls

zhjwpku · 2025-01-25T14:44:31Z

src/iceberg/type.h

+  [[nodiscard]] virtual bool is_primitive() const = 0;
+
+  /// \brief Is this a nested type (may have child fields)?
+  [[nodiscard]] virtual bool is_nested() const = 0;


Isn't this redundant with is_primitive()?

To me it's clearer to be able to say is_nested or is_primitive and they can just be implemented in terms of each other.

Ok, let's keep both.

zhjwpku · 2025-01-25T14:49:07Z

src/iceberg/type.h

+  bool Equals(const Type& other) const override;
+};
+
+/// \brief A data type representing a 32-bit (single precision) float.


32-bit IEEE 754 floating point

zhjwpku · 2025-01-25T14:49:33Z

src/iceberg/type.h

+  bool Equals(const Type& other) const override;
+};
+
+/// \brief A data type representing a 64-bit (double precision) float.


64-bit IEEE 754 floating point
It will great we just copy past the type description from the spec. The same applied to other types.

zhjwpku · 2025-01-25T15:06:02Z

src/iceberg/type.h

+  bool Equals(const Type& other) const override;
+
+ private:
+  int32_t length_;


iceberg-rust has the length of u64[1], int32_t can represent about 2 gigabytes, maybe we should change to int64_t to be compatible with other language implementation?

[1] https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/spec/datatypes.rs#L241

Java uses a 32 bit int. https://github.com/apache/iceberg/blob/d693f83b6ad6e306550d117ff0bf0014356500b2/api/src/main/java/org/apache/iceberg/types/Types.java#L360

Are the Iceberg maintainers interested in making the spec actually lay out these details? I've mostly copied Java since the spec itself is extremely vague.

The int32 is fine. No one should go beyond 2GB values for a single value.

zhjwpku · 2025-01-25T15:29:16Z

src/iceberg/type_fwd.h

+  kTime,
+  kTimestamp,
+  kTimestampTz,
+  kBinary,


nit: list these the same order as spec.

Is the spec actually canonical? It seems the implementations all disagree...

And the spec doesn't actually have any single listing of all the types (since nested types are split off), so I am electing to put the nested types first to be consistent with the spec (in as far as it defines any order)

Updated the order

wgtmac · 2025-01-27T10:14:31Z

src/iceberg/schema.cc

+
+#include "iceberg/schema.h"
+
+#include <format>


This is redundant.

Isn't it good practice to IWYU?

I'm not that familiar with IWYU yet...

We use std::format in the .cc file so we should include format, even if other headers happen to include it already

wgtmac · 2025-01-27T10:27:15Z

src/iceberg/util/formatter.h

+
+#pragma once
+
+/// \file iceberg/util/formatter.h


Should this file be renamed to formatter_internal.h? We don't have to export and install this file.

FYI: I have followed the same pattern from Apache Arrow: https://github.com/apache/iceberg-cpp/blob/main/cmake_modules/IcebergBuildUtils.cmake#L251

No, you do need this file externally if you yourself wish to use std::format.

wgtmac · 2025-01-27T12:10:44Z

src/iceberg/type.h

+};
+
+/// \brief A data type that has child fields.
+class ICEBERG_EXPORT NestedType : public Type {


Do we need a function for getting number of fields?

fields().size(), or do you think a separate accessor is useful?

That's enough I think.

wgtmac · 2025-01-27T12:12:27Z

src/iceberg/type.h

+  /// \brief Get a field by index.
+  [[nodiscard]] virtual std::optional<std::reference_wrapper<const SchemaField>>
+  GetFieldByIndex(int32_t index) const = 0;
+  /// \brief Get a field by name.


Is this case sensitive?

wgtmac · 2025-01-27T12:17:03Z

src/iceberg/type.cc

+  for (const auto& field : fields_) {
+    auto [it, inserted] = field_id_to_index_.try_emplace(field.field_id(), index);
+    if (!inserted) {
+      throw std::runtime_error(


Should we define IcebergException or even a more specific exception to replace it?

Are we using exceptions in general? I am only using an exception here because this is a constructor. We could also hide the constructor, or change to assertions.

No, my point is that a more specific exception is better than the std::runtime_error.

BTW, should we build field_id_to_index_ lazily when the particular getter is called for the 1st time?

That's discussed above, but it complicates the implementation a lot

wgtmac · 2025-01-27T12:19:46Z

src/iceberg/type.cc

+
+TypeId StructType::type_id() const { return TypeId::kStruct; }
+std::string StructType::ToString() const {
+  std::string repr = "struct<\n";


Is there a specific reason to use a separate line for each sub-field? An alternative is to use the Hive-style where all fields are concatenated in a single line.

That gets messy IMO once there are more than a couple fields

Do we want to support restoring a Schema object by deserializing the formatted string?

I'd rather add JSON serializers than try to parse the repr

wgtmac · 2025-01-27T12:29:18Z

src/iceberg/type.cc

+}
+std::optional<std::reference_wrapper<const SchemaField>> StructType::GetFieldByName(
+    std::string_view name) const {
+  // TODO: what is the right behavior if there are duplicate names? (Are


It seems that the spec is unclear about this so the behavior is undefined and implementation-specific.

There seem to be a lot of spec issues...

Please consider the Java implementation as the reference one. There we raise a ValidationException when we see a duplicate name:

https://github.com/apache/iceberg/blob/507e2a9e39f93356bdc0dae1521bbdc46629681c/api/src/main/java/org/apache/iceberg/types/IndexByName.java#L199-L205

There seem to be a lot of spec issues...

@lidavidm Please elaborate

Is this in the written spec? Or is the canonical-ness of the Java implementation documented somewhere? Just so I know what to reference

Also see above e.g. #31 (comment)
#31 (comment)

possibly I overlooked something

The spec describes the table format itself, for the behavioral things, we tend to check the Java implementation (considered the reference implementation). If we would need to formalize all this as well, then the spec would become unreadable.

IMO, at least something like "duplicate field names are not allowed" should be part of the spec, no? I wouldn't really qualify that as "behavioral" any more than "decimal precision is limited to 38" is behavioral

Since Iceberg comes from the database world, not having duplicate names is more or less implied to me, but feel free to raise a PR to add this to the spec itself.

I was surprised to see that this is allowed:

>>> import pyarrow as pa >>> pa.table([ ... pa.array(["foo", "bar"]), ... pa.array(["Parrot", "Dog"]) ... ], names=['animals', 'animals']) pyarrow.Table animals: string animals: string ---- animals: [["foo","bar"]] animals: [["Parrot","Dog"]]

I was surprised to see that this is allowed:

I think Arrow is different. It is totally valid to project the same column more than once in an Arrow Table or RecordBatch. For example, returning query result via Arrow for select animals, animals from foo.bar;

wgtmac

+1

lidavidm · 2025-02-02T11:41:00Z

Is anything else required here?

zhjwpku · 2025-02-02T14:20:34Z

Is anything else required here?

+1 to merge this PR, thanks for working on this.

wgtmac · 2025-02-03T09:14:25Z

@Fokko @Xuanwo I think this is ready to merge. cc @gaborkaszab in case there is any comment.

gaborkaszab · 2025-02-03T09:23:47Z

@Fokko @Xuanwo I think this is ready to merge. cc @gaborkaszab in case there is any comment.

I got a bit busy with other stuff, so wouldn't block this PR for me to find time here. Thanks for these changes!

Fokko · 2025-02-03T16:13:31Z

src/iceberg/schema_field.cc

+  return std::format("{} ({}): {}{}", name_, field_id_, *type_,
+                     optional_ ? "" : " (required)");


Maybe it is just me, but I'm not a big fan of the implied (optional)

Fokko · 2025-02-03T16:18:52Z

src/iceberg/type.cc

+  }
+  return fields_[index];
+}
+std::optional<std::reference_wrapper<const SchemaField>> StructType::GetFieldByName(


We also want to distinguish upper- and lower-case, or start with case-sensitive here.

The docstring states this is case-sensitive already. Should we add something like Java's StructType#caseInsensitiveField?

Fokko

Looks like there is consensus, let's move this forward 🚀

lidavidm · 2025-02-04T00:30:12Z

Thanks @Fokko I'll follow up to correct those things

lidavidm commented Jan 16, 2025

View reviewed changes

src/iceberg/type_fwd.h Show resolved Hide resolved

lidavidm commented Jan 16, 2025

View reviewed changes

src/iceberg/schema.h Show resolved Hide resolved

Fokko reviewed Jan 16, 2025

View reviewed changes

src/iceberg/schema.h Show resolved Hide resolved

src/iceberg/type.h Outdated Show resolved Hide resolved

src/iceberg/type_fwd.h Show resolved Hide resolved

src/iceberg/type.h Outdated Show resolved Hide resolved

wgtmac reviewed Jan 17, 2025

View reviewed changes

src/iceberg/type_fwd.h Show resolved Hide resolved

src/iceberg/type.h Outdated Show resolved Hide resolved

src/iceberg/type.h Outdated Show resolved Hide resolved

src/iceberg/schema.h Show resolved Hide resolved

lidavidm force-pushed the schema branch from a2d1a9e to 9df1312 Compare January 17, 2025 07:24

gaborkaszab reviewed Jan 17, 2025

View reviewed changes

mapleFU reviewed Jan 20, 2025

View reviewed changes

lidavidm force-pushed the schema branch 3 times, most recently from e0d6e5e to a32ed60 Compare January 21, 2025 05:33

lidavidm force-pushed the schema branch 2 times, most recently from eb74e90 to 40a18b1 Compare January 21, 2025 06:01

Add headers for type/field/schema

3846d96

lidavidm force-pushed the schema branch from 40a18b1 to 3846d96 Compare January 21, 2025 06:19

fix clang

ce37966

lidavidm changed the title ~~WIP: Add headers for type/field/schema~~ Add data type/schema field/schema Jan 22, 2025

lidavidm marked this pull request as ready for review January 22, 2025 06:27

wgtmac reviewed Jan 22, 2025

View reviewed changes

lidavidm added 4 commits January 22, 2025 21:13

add more unit tests

91cd927

add more unit tests

85f1ade

depend on gmock

e609d5d

add field/schema tests

bbc18d5

raulcd reviewed Jan 24, 2025

View reviewed changes

mapleFU reviewed Jan 24, 2025

View reviewed changes

zhjwpku reviewed Jan 25, 2025

View reviewed changes

lidavidm added 2 commits January 26, 2025 18:58

updates

d2948b5

updates

22f734d

zhjwpku approved these changes Jan 27, 2025

View reviewed changes

wgtmac reviewed Jan 27, 2025

View reviewed changes

lidavidm added 3 commits January 27, 2025 08:21

nit

285f7d6

nit

35b73e4

nit

e4a5413

wgtmac approved these changes Jan 27, 2025

View reviewed changes

mapleFU approved these changes Feb 2, 2025

View reviewed changes

Fokko requested review from raulcd, Fokko and gaborkaszab February 3, 2025 16:05

Fokko reviewed Feb 3, 2025

View reviewed changes

Fokko approved these changes Feb 3, 2025

View reviewed changes

Fokko merged commit ebacc54 into apache:main Feb 3, 2025
6 checks passed

lidavidm deleted the schema branch February 4, 2025 00:43

		return std::format("{} ({}): {}{}", name_, field_id_, *type_,
		optional_ ? "" : " (required)");


		#include "iceberg/schema.h"

		#include <format>

Add data type/schema field/schema #31

Add data type/schema field/schema #31

Conversation

lidavidm commented Jan 16, 2025 • edited Loading

lidavidm commented Jan 16, 2025

lidavidm commented Jan 16, 2025

Fokko left a comment

Choose a reason for hiding this comment

lidavidm commented Jan 17, 2025

gaborkaszab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborkaszab Jan 17, 2025 • edited Loading

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

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

lidavidm commented Jan 20, 2025

lidavidm commented Jan 20, 2025

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

wgtmac commented Jan 20, 2025

mapleFU commented Jan 20, 2025

gaborkaszab commented Jan 20, 2025

lidavidm commented Jan 21, 2025 • edited Loading

lidavidm commented Jan 22, 2025

lidavidm commented Jan 22, 2025

wgtmac left a 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

lidavidm commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU left a 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

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

lidavidm commented Jan 16, 2025 •

edited

Loading

gaborkaszab Jan 17, 2025 •

edited

Loading

lidavidm commented Jan 21, 2025 •

edited

Loading