Merge pull request #175 from Maxxen/dev

Remove unaligned vertex accesses

.github/workflows/Linux.yml

@@ -95,7 +95,7 @@ jobs:
         openssl: 0
 
     - name: Setup vcpkg
-      uses: lukka/run-vcpkg@v11
+      uses: lukka/run-vcpkg@v11.1
       with:
         vcpkgGitCommitId: 501db0f17ef6df184fcdbfbe0f87cde2313b6ab1
 

.github/workflows/MainDistributionPipeline.yml

@@ -20,7 +20,7 @@ concurrency:
 jobs:
   duckdb-stable-build:
     name: Build extension binaries
-    uses: duckdb/duckdb/.github/workflows/_extension_distribution.yml@main
+    uses: duckdb/duckdb/.github/workflows/_extension_distribution.yml@60ddc316ca0c1585f14d55aa73f9db59d8fc05d1
     with:
       duckdb_version: v0.9.1
       extension_name: spatial

spatial/include/spatial/core/geometry/geometry.hpp

@@ -104,8 +104,7 @@ class Point {
 	}
 	string ToString() const;
 	bool IsEmpty() const;
-	Vertex &GetVertex();
-	const Vertex &GetVertex() const;
+	Vertex GetVertex() const;
 
 	const VertexVector &Vertices() const {
 		return vertices;

spatial/include/spatial/core/geometry/vertex_vector.hpp

@@ -66,39 +66,18 @@ class VertexVector {
 public:
 	uint32_t count;
 	uint32_t capacity;
-	Vertex *data;
+	data_ptr_t data;
 
-	explicit VertexVector(Vertex *data, uint32_t count, uint32_t capacity)
+	explicit VertexVector(data_ptr_t data, uint32_t count, uint32_t capacity)
 	    : count(count), capacity(capacity), data(data) {
 	}
 
 	// Create a VertexVector from an already existing buffer
-	static VertexVector FromBuffer(Vertex *buffer, uint32_t count) {
+	static VertexVector FromBuffer(data_ptr_t buffer, uint32_t count) {
 		VertexVector array(buffer, count, count);
 		return array;
 	}
 
-	inline Vertex &operator[](uint32_t index) const {
-		D_ASSERT(index < count);
-		return data[index];
-	}
-
-	inline Vertex *begin() {
-		return data;
-	}
-
-	inline Vertex *end() {
-		return data + count;
-	}
-
-	const Vertex *begin() const {
-		return data;
-	}
-
-	const Vertex *end() const {
-		return data + count;
-	}
-
 	inline uint32_t Count() const {
 		return count;
 	}
@@ -109,7 +88,18 @@ class VertexVector {
 
 	inline void Add(const Vertex &v) {
 		D_ASSERT(count < capacity);
-		data[count++] = v;
+		Store<Vertex>(v, data + count * sizeof(Vertex));
+		count++;
+	}
+
+	inline void Set(uint32_t index, const Vertex &v) const {
+		D_ASSERT(index < count);
+		Store<Vertex>(v, data + index * sizeof(Vertex));
+	}
+
+	inline Vertex Get(uint32_t index) const {
+		D_ASSERT(index < count);
+		return Load<Vertex>(data + index * sizeof(Vertex));
 	}
 
 	// Returns the number of bytes that this VertexVector requires to be serialized
@@ -120,10 +110,6 @@ class VertexVector {
 	void Serialize(Cursor &cursor) const;
 	void SerializeAndUpdateBounds(Cursor &cursor, BoundingBox &bbox) const;
 
-	inline Vertex *Data() {
-		return data;
-	}
-
 	double Length() const;
 	double SignedArea() const;
 	double Area() const;

spatial/src/spatial/core/functions/cast/geometry_cast.cpp

@@ -46,7 +46,7 @@ static bool GeometryToPoint2DCast(Vector &source, Vector &result, idx_t count, C
 		if (point.IsEmpty()) {
 			throw CastException("Cannot cast empty point GEOMETRY to POINT_2D");
 		}
-		auto &vertex = point.GetVertex();
+		auto vertex = point.GetVertex();
 		return POINT_TYPE {vertex.x, vertex.y};
 	});
 	return true;
@@ -103,8 +103,8 @@ static bool GeometryToLineString2DCast(Vector &source, Vector &result, idx_t cou
 		ListVector::Reserve(result, total_coords);
 
 		for (idx_t i = 0; i < line_size; i++) {
-			x_data[entry.offset + i] = line.Vertices()[i].x;
-			y_data[entry.offset + i] = line.Vertices()[i].y;
+			x_data[entry.offset + i] = line.Vertices().Get(i).x;
+			y_data[entry.offset + i] = line.Vertices().Get(i).y;
 		}
 		return entry;
 	});
@@ -182,8 +182,8 @@ static bool GeometryToPolygon2DCast(Vector &source, Vector &result, idx_t count,
 			ring_entries[total_rings + ring_idx] = ring_entry;
 
 			for (idx_t j = 0; j < ring_size; j++) {
-				x_data[ring_entry.offset + j] = ring.data[j].x;
-				y_data[ring_entry.offset + j] = ring.data[j].y;
+				x_data[ring_entry.offset + j] = ring.Get(j).x;
+				y_data[ring_entry.offset + j] = ring.Get(j).y;
 			}
 			total_coords += ring_size;
 		}
@@ -217,16 +217,13 @@ static bool Box2DToGeometryCast(Vector &source, Vector &result, idx_t count, Cas
 
 		auto geom = lstate.factory.CreatePolygon(1, &capacity);
 		auto &shell = geom.Ring(0);
-		shell.data[0].x = minx;
-		shell.data[0].y = miny;
-		shell.data[1].x = maxx;
-		shell.data[1].y = miny;
-		shell.data[2].x = maxx;
-		shell.data[2].y = maxy;
-		shell.data[3].x = minx;
-		shell.data[3].y = maxy;
-		shell.data[4].x = minx;
-		shell.data[4].y = miny;
+
+		shell.Set(0, Vertex(minx, miny));
+		shell.Set(1, Vertex(maxx, miny));
+		shell.Set(2, Vertex(maxx, maxy));
+		shell.Set(3, Vertex(minx, maxy));
+		shell.Set(4, Vertex(minx, miny));
+
 		shell.count = 5;
 		return lstate.factory.Serialize(result, Geometry(geom));
 	});

spatial/src/spatial/core/functions/scalar/st_asgeojson.cpp

@@ -60,7 +60,7 @@ class JSONAllocator {
 static void VerticesToGeoJSON(const VertexVector &vertices, yyjson_mut_doc *doc, yyjson_mut_val *arr) {
 	// TODO: If the vertexvector is empty, do we null, add an empty array or a pair of NaN?
 	for (uint32_t i = 0; i < vertices.count; i++) {
-		auto &vertex = vertices[i];
+		auto vertex = vertices.Get(i);
 		auto coord = yyjson_mut_arr(doc);
 		yyjson_mut_arr_add_real(doc, coord, vertex.x);
 		yyjson_mut_arr_add_real(doc, coord, vertex.y);
@@ -75,7 +75,7 @@ static void ToGeoJSON(const Point &point, yyjson_mut_doc *doc, yyjson_mut_val *o
 	yyjson_mut_obj_add_val(doc, obj, "coordinates", coords);
 
 	if (!point.IsEmpty()) {
-		auto &vertex = point.GetVertex();
+		auto vertex = point.GetVertex();
 		yyjson_mut_arr_add_real(doc, coords, vertex.x);
 		yyjson_mut_arr_add_real(doc, coords, vertex.y);
 	}

spatial/src/spatial/core/functions/scalar/st_dump.cpp

@@ -23,7 +23,7 @@ static void DumpFunction(DataChunk &args, ExpressionState &state, Vector &result
 	idx_t total_geom_count = 0;
 	idx_t total_path_count = 0;
 
-	for(idx_t out_row_idx = 0; out_row_idx < count; out_row_idx++) {
+	for (idx_t out_row_idx = 0; out_row_idx < count; out_row_idx++) {
 		auto in_row_idx = geom_format.sel->get_index(out_row_idx);
 
 		if (!geom_format.validity.RowIsValid(in_row_idx)) {
@@ -39,38 +39,35 @@ static void DumpFunction(DataChunk &args, ExpressionState &state, Vector &result
 
 		stack.emplace_back(geometry, vector<int32_t>());
 
-		while(!stack.empty()) {
+		while (!stack.empty()) {
 			auto current = stack.back();
 			auto current_geom = std::get<0>(current);
 			auto current_path = std::get<1>(current);
 
 			stack.pop_back();
 
-			if(current_geom.Type() == GeometryType::MULTIPOINT) {
+			if (current_geom.Type() == GeometryType::MULTIPOINT) {
 				auto mpoint = current_geom.GetMultiPoint();
 				for (int32_t i = 0; i < mpoint.Count(); i++) {
 					auto path = current_path;
 					path.push_back(i + 1); // path is 1-indexed
 					stack.emplace_back(mpoint[i], path);
 				}
-			}
-			else if(current_geom.Type() == GeometryType::MULTILINESTRING) {
+			} else if (current_geom.Type() == GeometryType::MULTILINESTRING) {
 				auto mline = current_geom.GetMultiLineString();
 				for (int32_t i = 0; i < mline.Count(); i++) {
 					auto path = current_path;
 					path.push_back(i + 1);
 					stack.emplace_back(mline[i], path);
 				}
-			}
-			else if(current_geom.Type() == GeometryType::MULTIPOLYGON) {
+			} else if (current_geom.Type() == GeometryType::MULTIPOLYGON) {
 				auto mpoly = current_geom.GetMultiPolygon();
 				for (int32_t i = 0; i < mpoly.Count(); i++) {
 					auto path = current_path;
 					path.push_back(i + 1);
 					stack.emplace_back(mpoly[i], path);
 				}
-			}
-			else if (current_geom.Type() == GeometryType::GEOMETRYCOLLECTION) {
+			} else if (current_geom.Type() == GeometryType::GEOMETRYCOLLECTION) {
 				auto collection = current_geom.GetGeometryCollection();
 				for (int32_t i = 0; i < collection.Count(); i++) {
 					auto path = current_path;
@@ -105,7 +102,7 @@ static void DumpFunction(DataChunk &args, ExpressionState &state, Vector &result
 		auto &result_path_vec = result_list_children[1];
 
 		auto geom_data = FlatVector::GetData<string_t>(*result_geom_vec);
-		for(idx_t i = 0; i < geom_length; i++) {
+		for (idx_t i = 0; i < geom_length; i++) {
 			// Write the geometry
 			auto &item_blob = std::get<0>(items[i]);
 			geom_data[geom_offset + i] = lstate.factory.Serialize(*result_geom_vec, item_blob);
@@ -128,24 +125,25 @@ static void DumpFunction(DataChunk &args, ExpressionState &state, Vector &result
 			auto &path_data_vec = ListVector::GetEntry(*result_path_vec);
 			auto path_data = FlatVector::GetData<int32_t>(path_data_vec);
 
-			for(idx_t j = 0; j < path_length; j++) {
+			for (idx_t j = 0; j < path_length; j++) {
 				path_data[path_offset + j] = path[j];
 			}
 		}
 	}
 
-	if(count == 1) {
+	if (count == 1) {
 		result.SetVectorType(VectorType::CONSTANT_VECTOR);
 	}
 }
 
 void CoreScalarFunctions::RegisterStDump(DatabaseInstance &db) {
 	ScalarFunctionSet set("ST_Dump");
 
-	set.AddFunction(ScalarFunction({GeoTypes::GEOMETRY()},
-	                               LogicalType::LIST(LogicalType::STRUCT({{"geom", GeoTypes::GEOMETRY()}, {"path", LogicalType::LIST(LogicalType::INTEGER)}})),
-	                               DumpFunction,
-	                               nullptr, nullptr, nullptr, GeometryFunctionLocalState::Init));
+	set.AddFunction(
+	    ScalarFunction({GeoTypes::GEOMETRY()},
+	                   LogicalType::LIST(LogicalType::STRUCT(
+	                       {{"geom", GeoTypes::GEOMETRY()}, {"path", LogicalType::LIST(LogicalType::INTEGER)}})),
+	                   DumpFunction, nullptr, nullptr, nullptr, GeometryFunctionLocalState::Init));
 
 	ExtensionUtil::RegisterFunction(db, set);
 }

spatial/src/spatial/core/functions/scalar/st_endpoint.cpp

@@ -80,7 +80,7 @@ static void GeometryEndPointFunction(DataChunk &args, ExpressionState &state, Ve
 			    return string_t();
 		    }
 
-		    auto &point = line.Vertices()[point_count - 1];
+		    auto point = line.Vertices().Get(point_count - 1);
 		    return lstate.factory.Serialize(result, Geometry(lstate.factory.CreatePoint(point.x, point.y)));
 	    });
 }

spatial/src/spatial/core/functions/scalar/st_exteriorring.cpp

@@ -114,7 +114,7 @@ static void GeometryExteriorRingFunction(DataChunk &args, ExpressionState &state
 
 		    auto line = lstate.factory.CreateLineString(num_points);
 		    for (uint32_t i = 0; i < num_points; i++) {
-			    line.Vertices().Add(shell[i]);
+			    line.Vertices().Add(shell.Get(i));
 		    }
 		    return lstate.factory.Serialize(result, Geometry(line));
 	    });

spatial/src/spatial/core/functions/scalar/st_flipcoordinates.cpp

@@ -155,7 +155,9 @@ static void BoxFlipCoordinatesFunction(DataChunk &args, ExpressionState &state,
 //------------------------------------------------------------------------------
 static void FlipVertexVector(VertexVector &vertices) {
 	for (idx_t i = 0; i < vertices.count; i++) {
-		std::swap(vertices[i].x, vertices[i].y);
+		auto vertex = vertices.Get(i);
+		std::swap(vertex.x, vertex.y);
+		vertices.Set(i, vertex);
 	}
 }
 static void FlipGeometry(Point &point) {

spatial/src/spatial/core/functions/scalar/st_makepolygon.cpp

@@ -35,7 +35,7 @@ static void MakePolygonFromRingsFunction(DataChunk &args, ExpressionState &state
 		    }
 
 		    auto &shell_verts = shell.Vertices();
-		    if (shell_verts[0] != shell_verts[shell_vert_count - 1]) {
+		    if (!shell_verts.IsClosed()) {
 			    throw InvalidInputException(
 			        "ST_MakePolygon shell must be closed (first and last vertex must be equal)");
 		    }
@@ -70,7 +70,7 @@ static void MakePolygonFromRingsFunction(DataChunk &args, ExpressionState &state
 			    }
 
 			    auto &ring_verts = hole.Vertices();
-			    if (ring_verts[0] != ring_verts[hole_count - 1]) {
+			    if (!ring_verts.IsClosed()) {
 				    throw InvalidInputException(StringUtil::Format(
 				        "ST_MakePolygon hole #%lu must be closed (first and last vertex must be equal)", hole_idx + 1));
 			    }
@@ -84,8 +84,9 @@ static void MakePolygonFromRingsFunction(DataChunk &args, ExpressionState &state
 		    for (idx_t ring_idx = 0; ring_idx < rings.size(); ring_idx++) {
 			    auto &new_ring = rings[ring_idx];
 			    auto &poly_ring = polygon.Ring(ring_idx);
-			    for (auto &v : new_ring.Vertices()) {
-				    poly_ring.Add(v);
+
+			    for (auto i = 0; i < new_ring.Vertices().Count(); i++) {
+				    poly_ring.Add(new_ring.Vertices().Get(i));
 			    }
 		    }
 
@@ -111,13 +112,13 @@ static void MakePolygonFromShellFunction(DataChunk &args, ExpressionState &state
 		}
 
 		auto &line_verts = line.Vertices();
-		if (line_verts[0] != line_verts[line_count - 1]) {
+		if (!line_verts.IsClosed()) {
 			throw InvalidInputException("ST_MakePolygon shell must be closed (first and last vertex must be equal)");
 		}
 
 		auto polygon = lstate.factory.CreatePolygon(1, &line_count);
-		for (auto &v : line_verts) {
-			polygon.Shell().Add(v);
+		for (uint32_t i = 0; i < line_count; i++) {
+			polygon.Shell().Add(line_verts.Get(i));
 		}
 
 		return lstate.factory.Serialize(result, Geometry(polygon));

spatial/src/spatial/core/functions/scalar/st_perimeter.cpp

@@ -76,8 +76,8 @@ static double PolygonPerimeter(const Polygon &poly) {
 	double perimeter = 0;
 	for (auto &ring : poly.Rings()) {
 		for (uint32_t i = 0; i < ring.Count() - 1; i++) {
-			auto &v1 = ring[i];
-			auto &v2 = ring[i + 1];
+			auto v1 = ring.Get(i);
+			auto v2 = ring.Get(i + 1);
 			perimeter += std::sqrt(std::pow(v1.x - v2.x, 2) + std::pow(v1.y - v2.y, 2));
 		}
 	}

spatial/src/spatial/core/functions/scalar/st_pointn.cpp

@@ -90,7 +90,7 @@ static void GeometryPointNFunction(DataChunk &args, ExpressionState &state, Vect
 		    }
 
 		    auto actual_index = index < 0 ? point_count + index : index - 1;
-		    auto &point = line.Vertices()[actual_index];
+		    auto point = line.Vertices().Get(actual_index);
 		    return lstate.factory.Serialize(result, Geometry(lstate.factory.CreatePoint(point.x, point.y)));
 	    });
 }

spatial/src/spatial/core/functions/scalar/st_startpoint.cpp

@@ -80,7 +80,7 @@ static void GeometryStartPointFunction(DataChunk &args, ExpressionState &state,
 			    return string_t();
 		    }
 
-		    auto &point = line.Vertices()[0];
+		    auto point = line.Vertices().Get(0);
 		    return lstate.factory.Serialize(result, Geometry(lstate.factory.CreatePoint(point.x, point.y)));
 	    });
 }