Small improvements #493

dpxcc · 2024-12-14T01:23:19Z

Make pgduckdb/pgduckdb_types.hpp self-contained
Enable InvokeCPPFunc to call functions without arguments

dpxcc · 2024-12-14T01:30:41Z

Another issue I noticed is that the code isn't correctly formatted - running make format results in changes to code

Y--

Thanks for contribution!

Not opposed to these changes, but not clear what it solve? Would you mind clarifying in the PR description? Thanks!

@dpxcc

In #98 we started using the same version of clang-format that DuckDB uses. That version is pretty ancient though. So it misses a bunch of features that we might want to use, and also has a bunch of bugs which result in sub-optimal formatting. This updates `clang-format` to the latest version, and while we're at it we also update `ruff`. To be able to reformat all existing files, this also adds a `format-all` target to our `Makefile`. The problem was called out by @dpxcc in #493, but I've been also running into this myself for a while.

dpxcc · 2024-12-16T19:25:53Z

@Y-- Sorry, I think I forgot to submit my inline comments :(

dpxcc · 2024-12-14T01:27:35Z

include/pgduckdb/pgduckdb_types.hpp

@@ -1,5 +1,6 @@
 #pragma once

+#include "duckdb/common/types/data_chunk.hpp"


This header file used several DuckDB definitions, which should be included directly in the header file
duckdb/common/types/data_chunk.hpp includes them all

Without this fix, all .cpp files must include duckdb/common/types/data_chunk.hpp (or any file that includes it) before including pgduckdb/pgduckdb_types.hpp
This is undesirable, and the header file should be self-contained

Fair, but in this case we should clean the cpp files where pgduckdb_types.hpp is included to remove the extra duckdb includes no?

We don't include data_chunk.hpp anywhere else. That brings two questions:

Should this header simply include duckdb.hpp instead of a specific header?

@Y-- your suggestion for removal then becomes: should we remove duckdb.hpp from cpp files that already include pgduckdb_types.hpp?

Right I think if we're going to make this header self-contained. Or maybe better we should forward-declare the various DuckDB classes we're using in the functions below?

Yes and yes :-)

The convention in duckdb codebase seems to favor including specific headers rather than the entire duckdb.hpp
In my experience, using specific headers reduces compile time significantly, often by a factor of 3 (e.g., 1.8s -> 0.6s per file)

@dpxcc can you do this? then we can merge this PR imo.

Forgot to submit my comments..
Could you check my previous comments?
If you feel strongly to include entire duckdb.hpp instead of specific headers in pg_duckdb, I can proceed with the proposed changes

I'm fine with the specific headers (and definitely agree with your point, it's much better)

dpxcc · 2024-12-14T01:28:33Z

include/pgduckdb/utility/cpp_wrapper.hpp

@@ -30,7 +30,7 @@ __CPPFunctionGuard__(const char *func_name, FuncArgs... args) {

 }

-#define InvokeCPPFunc(FUNC, ...) pgduckdb::__CPPFunctionGuard__<decltype(&FUNC), &FUNC>(__FUNCTION__, __VA_ARGS__)
+#define InvokeCPPFunc(FUNC, ...) pgduckdb::__CPPFunctionGuard__<decltype(&FUNC), &FUNC>(__FUNCTION__, ##__VA_ARGS__)


The proposed change enables the InvokeCPPFunc macro to be used with empty __VA_ARGS__

Without this fix, InvokeCPPFunc(FuncWithoutArg) cannot be used to call a function without input arguments

Indeed, but we don't need it for now. Ideally I'd prefer to change it only when we have this use case. What do you think?

I'd think it makes sense to just merge this, it seems more like an oversight to not have this macro support functions with no parameters.

To provide some context, this issue arises because pg_mooncake, built on pg_duckdb, uses InvokeCPPFunc with a no-parameter function. We've fixed it in pg_mooncake and would like to upstream the fix

Ah nice, thanks for the context, that's very helpful!
Don't hesitate to provide feedback or reach out to us if we can be more friendly for such cases in the future :-)

wuputah · 2024-12-16T19:30:22Z

heya, the PR name + description is what gets committed when we do the squash merge, so if you could improve that, it would be most appreciated. Thanks!

dpxcc · 2024-12-16T19:33:20Z

@wuputah Done. Thanks for the information!

1. Make `pgduckdb/pgduckdb_types.hpp` self-contained 2. Enable `InvokeCPPFunc` to call functions without arguments

JelteF

@dpxcc general feedback. Since these changes are totally separate. I'd normally prefer to get them as two PRs. Mainly, so that if one of the changes turns out to be contentious while the other is not, we can merge the non contentious part without it being blocked on the other unrelated part.

No need to split up this one though, unless you feel like doing so.

dpxcc · 2024-12-18T19:33:35Z

@dpxcc general feedback. Since these changes are totally separate. I'd normally prefer to get them as two PRs. Mainly, so that if one of the changes turns out to be contentious while the other is not, we can merge the non contentious part without it being blocked on the other unrelated part.

No need to split up this one though, unless you feel like doing so.

Thanks for the information! I will follow this guideline in the future

dpxcc marked this pull request as ready for review December 14, 2024 01:28

Y-- reviewed Dec 16, 2024

View reviewed changes

JelteF mentioned this pull request Dec 16, 2024

Bump clang-format and ruff versions to latest #496

Merged

dpxcc commented Dec 16, 2024

View reviewed changes

dpxcc force-pushed the main branch 2 times, most recently from d1a2548 to 93d5e3f Compare December 17, 2024 01:44

Small improvements

2352ea2

1. Make `pgduckdb/pgduckdb_types.hpp` self-contained 2. Enable `InvokeCPPFunc` to call functions without arguments

dpxcc force-pushed the main branch from 93d5e3f to 2352ea2 Compare December 17, 2024 01:47

JelteF reviewed Dec 18, 2024

View reviewed changes

JelteF enabled auto-merge (squash) December 20, 2024 14:46

JelteF approved these changes Dec 20, 2024

View reviewed changes

JelteF merged commit 3b34bee into duckdb:main Dec 20, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small improvements #493

Small improvements #493

dpxcc commented Dec 14, 2024 •

edited

Loading

dpxcc commented Dec 14, 2024

Y-- left a comment

dpxcc commented Dec 16, 2024 •

edited

Loading

dpxcc Dec 14, 2024 •

edited

Loading

dpxcc Dec 16, 2024

Y-- Dec 18, 2024

JelteF Dec 18, 2024

Y-- Dec 18, 2024

Y-- Dec 18, 2024

dpxcc Dec 18, 2024

JelteF Dec 19, 2024 •

edited

Loading

dpxcc Dec 19, 2024

Y-- Dec 19, 2024

dpxcc Dec 14, 2024

dpxcc Dec 16, 2024

Y-- Dec 18, 2024

JelteF Dec 18, 2024

dpxcc Dec 18, 2024

Y-- Dec 19, 2024

wuputah commented Dec 16, 2024

dpxcc commented Dec 16, 2024

JelteF left a comment •

edited

Loading

dpxcc commented Dec 18, 2024

		@@ -1,5 +1,6 @@
		#pragma once

		#include "duckdb/common/types/data_chunk.hpp"

Small improvements #493

Small improvements #493

Conversation

dpxcc commented Dec 14, 2024 • edited Loading

dpxcc commented Dec 14, 2024

Y-- left a comment

Choose a reason for hiding this comment

dpxcc commented Dec 16, 2024 • edited Loading

dpxcc Dec 14, 2024 • 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

JelteF Dec 19, 2024 • 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

wuputah commented Dec 16, 2024

dpxcc commented Dec 16, 2024

JelteF left a comment • edited Loading

Choose a reason for hiding this comment

dpxcc commented Dec 18, 2024

dpxcc commented Dec 14, 2024 •

edited

Loading

dpxcc commented Dec 16, 2024 •

edited

Loading

dpxcc Dec 14, 2024 •

edited

Loading

JelteF Dec 19, 2024 •

edited

Loading

JelteF left a comment •

edited

Loading