Move BindCreateIndex from Catalog to Binder

[philippmd]

Currently BindCreateIndex is a method on the Catalog.
However, it doesn't modify or even read the catalog, instead taking a pointer to the Binder.

So I would think this is method fits better on the Binder?

Let me know what you think :)

PS: Sorry for my autoformatter re-ordering imports. I can ofc revert that.

[Tishj]

This is a virtual method, I would assume that has a reason

	virtual unique_ptr<LogicalOperator> BindCreateIndex(Binder &binder, CreateStatement &stmt, TableCatalogEntry &table,
	                                                    unique_ptr<LogicalOperator> plan) = 0;

The behavior can be overridden by Catalog extensions, which is not possible if this is a method on the Binder

[philippmd]

And I guess there is no way to figure out if such an Extension exists?

Are Binder Extensions a thing as well? In that case we could just keep the virtual.

[Mytherin]

Thanks for the PR! As mentioned, this method is in the catalog on purpose to facilitate pluggable indexes in catalogs. You can see in the CI runs that this change breaks several existing. extensions. For example, this is used in the Postgres extension. We recommend looking at the CI on your own fork prior to opening a PR in DuckDB.

As an alternative we could perhaps move the logic of DuckCatalog::BindCreateIndex into the Binder (e.g. Binder::BindCreateIndex) - and then call that function by DuckCatalog::BindCreateIndex. That way no extension functionality is lost.

[philippmd]

We recommend looking at the CI on your own fork prior to opening a PR in DuckDB.

Thanks! Will do that in the future; I only ran make unit before.

As an alternative we could perhaps move the logic of DuckCatalog::BindCreateIndex into the Binder (e.g. Binder::BindCreateIndex) - and then call that function by DuckCatalog::BindCreateIndex. That way no extension functionality is lost.

I think that would be good yes. Apart from that I would also be happy to move the logic to from DuckCatalog to Catalog, such that it's no longer pure virtual.
This would allow other implementors of Catalog to use it as a default, without having to copy the method from DuckCatalog to their own implementation. However it seems that there is no precendence for similar methods on Catalog.

What do you think?

[Mytherin]

The current changeset looks good to me - thanks!

[Mytherin]

Turns out this was breaking the various catalog extensions still - but this was missed because the extension API was broken for a different reason (the xz github repo being removed). Reverting for now in #11478

[Mytherin]

I’ve reverted this PR because of broken CI in #11478, feel free to send a new PR that applies this change

[philippmd]

Thanks and sorry for breaking things in the first place.

I'll investigate why and how they broke since I still believe that it would be nice if there was a default implementation of BindCreateIndex that Catalog extensions could use (without copying code around)

[carlopi]

I think your PR + carlopi@ad28d1f could possibly work together, since then it will retain the dynamic dispatch, but not certain.

Also note that currently CI for building postgres and sqlite is not yet properly functional, probably it will be after #11486 is merged.