Nullice

[Joe-Abraham]

No description provided.

[Copilot]

Pull Request Overview

This PR introduces several major enhancements and fixes to the Velox codebase, including new Spark SQL functions, type system improvements, and various bug fixes.

Key changes:

• Adds new Spark SQL functions: to_json, timestampadd, and enhanced split function with fast path optimization
• Improves type system with new valueToString method and utility functions for better string representation
• Fixes map filter function to support FlatMapVector encoding and resolves element_at caching issues

Reviewed Changes

Copilot reviewed 214 out of 215 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
velox/type/Type.h	Adds valueToString template method and new enum parameters for type system
velox/functions/sparksql/ToJson.h	New comprehensive ToJson function implementation with timezone support
velox/functions/sparksql/Split.h	Enhanced split function with fast path optimization for simple delimiters
velox/functions/prestosql/FilterFunctions.cpp	Major enhancement to map_filter supporting FlatMapVector encoding
velox/functions/lib/SubscriptUtil.cpp	Fixes element_at caching bug with non-zero offset handling

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[Copilot]

The error message should be more descriptive. Consider changing to "Input type HUGEINT must be a decimal type" to provide clearer context about what input is being validated.

Suggested change

	VELOX_DCHECK(input.type()->isDecimal(), "HUGEINT must be a decimal type.");
	VELOX_DCHECK(input.type()->isDecimal(), "Input type HUGEINT must be a decimal type.");

[Copilot]

Using std::stoi without error handling can throw exceptions for invalid input. Since this is checking octal validity, consider using a safer parsing approach or add exception handling.

Suggested change

	decimal = std::stoi(str + 1, nullptr, 8);
	int value = 0;
	for (int i = 1; i < size; ++i) {
	value = value * 8 + (str[i] - '0');
	}
	if (value > 0x7F) { // 177 octal == 127 decimal
	return false;
	}
	decimal = static_cast<char>(value);

[Copilot]

Same issue as above - std::stoi can throw exceptions. Add proper error handling or use a safer parsing method.

Suggested change

	decimal = std::stoi(str + 1, nullptr, 8);
	try {
	if (std::stoi(str + 1) > 177) {
	return false;
	}
	decimal = std::stoi(str + 1, nullptr, 8);
	} catch (const std::invalid_argument&) {
	return false;
	} catch (const std::out_of_range&) {
	return false;
	}

[Copilot]

[nitpick] Consider using vector_size_t instead of int for consistency with Velox codebase conventions for vector indexing.

Suggested change

	for (int i = 0; i < decodedMap.size(); i++) {
	for (vector_size_t i = 0; i < decodedMap.size(); i++) {

[Copilot]

The detail namespace and its classes/functions lack documentation. Consider adding brief comments explaining the purpose of the JsonOptions struct and helper functions.

Suggested change

	// Internal namespace containing helper structs and functions for JSON serialization.
	namespace detail {
	// Options for controlling JSON serialization behavior.
	struct JsonOptions {
	// Time zone to use for formatting date/time values.
	const tz::TimeZone* timeZone;
	// Whether to ignore null fields during JSON generation.
	const bool ignoreNullFields{true};
	};
	// Appends a floating-point value to the result string, handling special cases for map keys and non-finite values.