Skip to content

Commit 7d8d228

Browse files
rustyconoverrustyconover
committed
fix cleanups
1 parent de42d33 commit 7d8d228

File tree

2 files changed

+60
-71
lines changed

2 files changed

+60
-71
lines changed

‎src/textplot_extension.cpp‎

Lines changed: 54 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,7 @@ static unique_ptr<FunctionData> TextplotDensityBind(ClientContext &context, Scal
4848
throw BinderException("tp_density takes at least one argument");
4949
}
5050

51-
auto source_type = LogicalType::LIST(LogicalType::DOUBLE);
52-
auto &first_arg = arguments[0]->return_type;
51+
const auto &first_arg = arguments[0]->return_type;
5352
if (!first_arg.IsNested() || first_arg.InternalType() != PhysicalType::LIST ||
5453
!ListType::GetChildType(first_arg).IsNumeric()) {
5554
throw InvalidTypeException("tp_density first argument must be a list of numeric values");
@@ -59,22 +58,22 @@ static unique_ptr<FunctionData> TextplotDensityBind(ClientContext &context, Scal
5958
int64_t width = 20;
6059
std::vector<std::string> graph_characters;
6160
string marker_char;
62-
string style = "";
61+
string style;
6362

6463
for (idx_t i = 1; i < arguments.size(); i++) {
65-
auto &arg = arguments[i];
64+
const auto &arg = arguments[i];
6665
if (arg->HasParameter()) {
6766
throw ParameterNotResolvedException();
6867
}
6968
if (!arg->IsFoldable()) {
7069
throw BinderException("tp_density: arguments must be constant");
7170
}
72-
auto &alias = arg->GetAlias();
71+
const auto &alias = arg->GetAlias();
7372
if (alias == "width") {
7473
if (!arg->return_type.IsIntegral()) {
7574
throw BinderException("tp_density: 'width' argument must be an integer");
7675
}
77-
auto eval_result = ExpressionExecutor::EvaluateScalar(context, *arg);
76+
const auto eval_result = ExpressionExecutor::EvaluateScalar(context, *arg);
7877
width = eval_result.CastAs(context, LogicalType::UBIGINT).GetValue<uint64_t>();
7978
} else if (alias == "marker") {
8079
if (arg->return_type.id() != LogicalTypeId::VARCHAR) {
@@ -86,16 +85,14 @@ static unique_ptr<FunctionData> TextplotDensityBind(ClientContext &context, Scal
8685
throw BinderException("tp_density: 'graph_chars' argument must be a VARCHAR");
8786
}
8887

89-
child_list_t<LogicalType> children;
90-
9188
if (arg->return_type.InternalType() != PhysicalType::LIST) {
9289
throw BinderException(
9390
StringUtil::Format("tp_density: 'graph_chars' argument must be a list of strings it is %s",
9491
arg->return_type.ToString()));
9592
}
9693

97-
auto list_children = ListValue::GetChildren(ExpressionExecutor::EvaluateScalar(context, *arg));
98-
for (auto &list_item : list_children) {
94+
const auto list_children = ListValue::GetChildren(ExpressionExecutor::EvaluateScalar(context, *arg));
95+
for (const auto &list_item : list_children) {
9996
// These should also be lists.
10097
if (list_item.type() != LogicalType::VARCHAR) {
10198
throw BinderException(
@@ -121,8 +118,7 @@ static unique_ptr<FunctionData> TextplotDensityBind(ClientContext &context, Scal
121118

122119
if (!style.empty()) {
123120
// lookup the style in density_sets
124-
auto it = density_sets.find(style);
125-
if (it != density_sets.end()) {
121+
if (const auto it = density_sets.find(style); it != density_sets.end()) {
126122
graph_characters = it->second;
127123
} else {
128124
throw BinderException(StringUtil::Format("tp_density: Unknown style '%s'", style));
@@ -132,23 +128,23 @@ static unique_ptr<FunctionData> TextplotDensityBind(ClientContext &context, Scal
132128
}
133129

134130
inline void TextplotDensity(DataChunk &args, ExpressionState &state, Vector &result) {
135-
auto &func_expr = state.expr.Cast<BoundFunctionExpression>();
131+
const auto &func_expr = state.expr.Cast<BoundFunctionExpression>();
136132
const auto &bind_data = func_expr.bind_info->Cast<TextplotDensityBindData>();
137133

138134
auto &value_vector = args.data[0];
139135
Vector input_data(LogicalType::LIST(LogicalType::DOUBLE));
140136
VectorOperations::Cast(state.GetContext(), value_vector, input_data, args.size());
141137

142-
// auto list_entries = FlatVector::GetData<list_entry_t>(input_data);
143138
auto &child_data = ListVector::GetEntry(input_data);
144139
auto source_data = FlatVector::GetData<double>(child_data);
145140

146141
double markerValue = std::nan("");
147142

148143
UnaryExecutor::Execute<list_entry_t, string_t>(input_data, result, args.size(), [&](list_entry_t values) {
149144
std::vector<double> data_items;
145+
data_items.reserve(values.length);
150146

151-
for (idx_t i = values.offset; i < values.offset + values.length; i++) {
147+
for (auto i = values.offset; i < values.offset + values.length; i++) {
152148
data_items.push_back(source_data[i]);
153149
}
154150

@@ -157,53 +153,51 @@ inline void TextplotDensity(DataChunk &args, ExpressionState &state, Vector &res
157153
}
158154

159155
// Find min and max values
160-
auto minmax = std::minmax_element(data_items.begin(), data_items.end());
161-
double minVal = *minmax.first;
162-
double maxVal = *minmax.second;
156+
const auto minmax = std::minmax_element(data_items.cbegin(), data_items.cend());
157+
const double minVal = *minmax.first;
158+
const double maxVal = *minmax.second;
163159

164160
if (minVal == maxVal) {
165161
// All values are the same - use max density character
166-
auto maxChar = bind_data.density_chars.back();
167-
vector<string> output_items;
168-
for (idx_t i = 0; i < bind_data.width; i++) {
169-
output_items.push_back(maxChar);
170-
}
162+
const auto &maxChar = bind_data.density_chars.back();
163+
std::vector<string> output_items(bind_data.width, maxChar);
171164

172165
// Add marker if value matches
173-
if (!std::isnan(markerValue) && std::abs(minVal - markerValue) < 1e-10) {
174-
for (int i = 0; i < bind_data.width; i++) {
175-
output_items[i] = bind_data.marker_char;
176-
}
166+
if (!std::isnan(markerValue) && std::abs(minVal - markerValue) < 1e-10 && !bind_data.marker_char.empty()) {
167+
std::fill(output_items.begin(), output_items.end(), bind_data.marker_char);
177168
}
178169

179170
// Now join all of the items without commas
180-
std::string output_result = StringUtil::Join(output_items, "");
171+
std::string output_result;
172+
for (const auto &item : output_items) {
173+
output_result += item;
174+
}
181175
return StringVector::AddString(result, output_result);
182176
}
183177

184178
// Create histogram bins
185179
std::vector<int> bins(bind_data.width, 0);
186-
double range = maxVal - minVal;
187-
double binWidth = range / bind_data.width;
180+
const double range = maxVal - minVal;
181+
const double binWidth = range / bind_data.width;
188182

189183
// Count values in each bin
190-
for (double val : data_items) {
191-
int binIndex = static_cast<int>((val - minVal) / binWidth);
184+
for (const double val : data_items) {
185+
auto binIndex = static_cast<int>((val - minVal) / binWidth);
192186
if (binIndex >= bind_data.width)
193187
binIndex = bind_data.width - 1; // Handle edge case
194188
bins[binIndex]++;
195189
}
196190

197191
// Find max count for scaling
198-
int maxCount = *std::max_element(bins.begin(), bins.end());
192+
const int maxCount = *std::max_element(bins.cbegin(), bins.cend());
199193
if (maxCount == 0) {
200-
auto maxChar = bind_data.density_chars.front();
201-
vector<string> output_items;
202-
for (idx_t i = 0; i < bind_data.width; i++) {
203-
output_items.push_back(maxChar);
194+
const auto &maxChar = bind_data.density_chars.front();
195+
std::string output_result;
196+
for (int64_t i = 0; i < bind_data.width; i++) {
197+
output_result += maxChar;
204198
}
205199

206-
return StringVector::AddString(result, StringUtil::Join(output_items, ""));
200+
return StringVector::AddString(result, output_result);
207201
}
208202

209203
// Determine marker position if specified
@@ -216,16 +210,16 @@ inline void TextplotDensity(DataChunk &args, ExpressionState &state, Vector &res
216210

217211
// Generate ASCII representation using provided character set
218212
std::string output_result;
219-
int numLevels = bind_data.density_chars.size() - 1;
213+
const int numLevels = bind_data.density_chars.size() - 1;
220214

221215
for (int i = 0; i < bind_data.width; i++) {
222216
// Check if this position should have a marker
223-
if (i == markerPos) {
217+
if (i == markerPos && !bind_data.marker_char.empty()) {
224218
output_result += bind_data.marker_char;
225219
} else {
226220
// Scale bin count to character range
227-
double normalized = static_cast<double>(bins[i]) / maxCount;
228-
int charIndex = static_cast<int>(normalized * numLevels + 0.5);
221+
const auto normalized = static_cast<double>(bins[i]) / maxCount;
222+
auto charIndex = static_cast<int>(normalized * numLevels + 0.5);
229223
charIndex = std::min(charIndex, numLevels);
230224
output_result += bind_data.density_chars[charIndex];
231225
}
@@ -269,7 +263,7 @@ struct TextplotBarBindData : public FunctionData {
269263

270264
private:
271265
string get_char(const string &color, const string &default_color, const string &shape) const {
272-
const std::unordered_map<std::string, std::string> *lookup_map;
266+
const std::unordered_map<std::string, std::string> *lookup_map = nullptr;
273267
if (shape == "square") {
274268
lookup_map = &emoji_squares;
275269
} else if (shape == "circle") {
@@ -281,8 +275,7 @@ struct TextplotBarBindData : public FunctionData {
281275
}
282276

283277
if (!color.empty()) {
284-
auto it = lookup_map->find(color);
285-
if (it != lookup_map->end()) {
278+
if (const auto it = lookup_map->find(color); it != lookup_map->end()) {
286279
return it->second;
287280
} else {
288281
throw BinderException(StringUtil::Format("tp_bar: Unknown color value '%s'", color));
@@ -293,7 +286,7 @@ struct TextplotBarBindData : public FunctionData {
293286
}
294287

295288
std::string get_threshold_color(double n, const string &default_color) const {
296-
if (thresholds.size() == 0) {
289+
if (thresholds.empty()) {
297290
return default_color;
298291
}
299292
for (const auto &t : thresholds) {
@@ -356,47 +349,42 @@ static unique_ptr<FunctionData> TextplotBarBind(ClientContext &context, ScalarFu
356349
vector<std::pair<double, string>> thresholds;
357350

358351
for (idx_t i = 1; i < arguments.size(); i++) {
359-
auto &arg = arguments[i];
352+
const auto &arg = arguments[i];
360353
if (arg->HasParameter()) {
361354
throw ParameterNotResolvedException();
362355
}
363356
if (!arg->IsFoldable()) {
364357
throw BinderException("tp_bar: arguments must be constant");
365358
}
366-
auto &alias = arg->GetAlias();
359+
const auto &alias = arg->GetAlias();
367360
if (alias == "min") {
368361
if (!arg->return_type.IsNumeric()) {
369362
throw BinderException("tp_bar: 'min' argument must be numeric");
370363
}
371-
auto eval_result = ExpressionExecutor::EvaluateScalar(context, *arg);
364+
const auto eval_result = ExpressionExecutor::EvaluateScalar(context, *arg);
372365
min = eval_result.CastAs(context, LogicalType::DOUBLE).GetValue<double>();
373366
} else if (alias == "max") {
374367
if (!arg->return_type.IsNumeric()) {
375368
throw BinderException("tp_bar: 'max' argument must be numeric");
376369
}
377-
auto eval_result = ExpressionExecutor::EvaluateScalar(context, *arg);
370+
const auto eval_result = ExpressionExecutor::EvaluateScalar(context, *arg);
378371
max = eval_result.CastAs(context, LogicalType::DOUBLE).GetValue<double>();
379372
} else if (alias == "thresholds") {
380-
child_list_t<LogicalType> children;
381-
children.push_back({"threshold", LogicalType::DOUBLE});
382-
children.push_back({"color", LogicalType::VARCHAR});
383-
auto threshold_record_type = LogicalType::STRUCT(children);
384-
385373
if (arg->return_type.InternalType() != PhysicalType::LIST) {
386374
throw BinderException(StringUtil::Format(
387375
"tp_bar: 'thresholds' argument must be a list of structs it is %s", arg->return_type.ToString()));
388376
}
389377

390-
auto list_children = ListValue::GetChildren(ExpressionExecutor::EvaluateScalar(context, *arg));
391-
for (auto &list_item : list_children) {
378+
const auto list_children = ListValue::GetChildren(ExpressionExecutor::EvaluateScalar(context, *arg));
379+
for (const auto &list_item : list_children) {
392380
// These should also be lists.
393381
if (list_item.type().InternalType() != PhysicalType::STRUCT) {
394382
throw BinderException(
395383
StringUtil::Format("tp_bar: 'thresholds' child must be a struct it is %s value is %s",
396384
list_item.type().ToString(), list_item.ToString()));
397385
}
398386
// Here you can extract the fields from the struct if needed.
399-
auto struct_fields = StructValue::GetChildren(list_item);
387+
const auto struct_fields = StructValue::GetChildren(list_item);
400388
if (struct_fields.size() != 2) {
401389
throw BinderException(StringUtil::Format(
402390
"tp_bar: 'thresholds' child struct must have 2 fields it has %d", struct_fields.size()));
@@ -406,8 +394,8 @@ static unique_ptr<FunctionData> TextplotBarBind(ClientContext &context, ScalarFu
406394
"tp_bar: 'thresholds' child struct field 'threshold' must be numeric it is %s",
407395
struct_fields[0].type().ToString()));
408396
}
409-
double threshold = struct_fields[0].CastAs(context, LogicalType::DOUBLE).GetValue<double>();
410-
string color = struct_fields[1].CastAs(context, LogicalType::VARCHAR).GetValue<string>();
397+
const double threshold = struct_fields[0].CastAs(context, LogicalType::DOUBLE).GetValue<double>();
398+
const string color = struct_fields[1].CastAs(context, LogicalType::VARCHAR).GetValue<string>();
411399
thresholds.emplace_back(threshold, color);
412400
}
413401
std::sort(thresholds.begin(), thresholds.end(), [](const auto &a, const auto &b) {
@@ -417,10 +405,10 @@ static unique_ptr<FunctionData> TextplotBarBind(ClientContext &context, ScalarFu
417405
if (!arg->return_type.IsIntegral()) {
418406
throw BinderException("tp_bar: 'width' argument must be an integer");
419407
}
420-
auto eval_result = ExpressionExecutor::EvaluateScalar(context, *arg);
408+
const auto eval_result = ExpressionExecutor::EvaluateScalar(context, *arg);
421409
width = eval_result.CastAs(context, LogicalType::UBIGINT).GetValue<uint64_t>();
422410
} else if (alias == "filled") {
423-
auto eval_result = ExpressionExecutor::EvaluateScalar(context, *arg);
411+
const auto eval_result = ExpressionExecutor::EvaluateScalar(context, *arg);
424412
filled = eval_result.CastAs(context, LogicalType::BOOLEAN).GetValue<bool>();
425413
} else if (alias == "on") {
426414
if (arg->return_type.id() != LogicalTypeId::VARCHAR) {
@@ -465,14 +453,15 @@ static unique_ptr<FunctionData> TextplotBarBind(ClientContext &context, ScalarFu
465453

466454
inline void TextplotBar(DataChunk &args, ExpressionState &state, Vector &result) {
467455
auto &value_vector = args.data[0];
468-
auto &func_expr = state.expr.Cast<BoundFunctionExpression>();
456+
const auto &func_expr = state.expr.Cast<BoundFunctionExpression>();
469457
const auto &bind_data = func_expr.bind_info->Cast<TextplotBarBindData>();
470458

471459
UnaryExecutor::Execute<double, string_t>(value_vector, result, args.size(), [&](double value) {
472-
auto proportion = std::clamp((value - bind_data.min) / (bind_data.max - bind_data.min), 0.0, 1.0);
473-
int64_t filled_blocks = static_cast<int64_t>(std::round(bind_data.width * proportion));
460+
const auto proportion = std::clamp((value - bind_data.min) / (bind_data.max - bind_data.min), 0.0, 1.0);
461+
const auto filled_blocks = static_cast<int64_t>(std::round(bind_data.width * proportion));
474462

475463
string bar;
464+
bar.reserve(bind_data.width * 4); // Reserve space for potential multi-byte characters
476465
for (int64_t i = 0; i < bind_data.width; i++) {
477466
if (bind_data.filled) {
478467
// Fill all blocks up to the proportion

‎test/sql/quack.test‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,12 @@ Catalog Error: Scalar Function with name textplot does not exist!
1212
require textplot
1313

1414
# Confirm the extension works
15-
query I
16-
SELECT textplot('Sam');
15+
query T
16+
SELECT tp_bar(0.7)
1717
----
18-
Textplot Sam 🐥
18+
🟥🟥🟥🟥🟥🟥🟥⬜⬜⬜
1919

20-
query I
21-
SELECT textplot_openssl_version('Michael') ILIKE 'Textplot Michael, my linked OpenSSL version is OpenSSL%';
20+
query T
21+
SELECT tp_density([1,2,3], width := 5);
2222
----
23-
true
23+
█ █ █

0 commit comments

Comments
 (0)