Should I use DeclarationToTable or DeclarationToTableAsync here?

Use DeclarationToTable. We have not historically exposed async methods to python and I don't see much benefit in doing so.

I am getting compile errors with this more compact option, so wrote it out with if/elses below (but will still look into the errors)

nogil? Or do you only add that when needed? I don't really know and am just looking at the difference between the two unwrap methods and hopefully learning something new :)

Ah, that's a good reminder that I still need to check where to add nogil in general. I suppose for the options it is less critical, but certainly for executing with eg DeclarationToTable I should certainly put that in a nogil block.

What is this for? Why use this over unwrap?

This is the pattern we had for the compute options classes, so started with that. But in the end it's not actually being used here, so can remove it then.

Sadly, I don't think options are printable. Declarations can be printed with DeclarationToString. We could probably add ToString to the options objects if needed but I don't think that is typical (e.g. CSV options, etc.)

Sorry, I should have added some comments to the WIP things. This was a reminder to see if we want a repr. But I agree that for the option classes here, that's not very important. Typically you only create them just to directly pass to a Declaration constructor (inspecting options created by someone else seems not a typical use case here)

Value equality of options objects (i.e. *self.unwrap() == *other.unwrap()) should generally be good enough I think.

@westonpace the ExecNodeOptions class in C++ does not define a operator== overload, so I don't think it allows such equality check? (or where you thinking about pure pointer equality?)

Anyway, that's also not very important here (if we would like to add support for pickling those objects (and declarations / execplans in general, that might become more important, but that is definitely out of scope for this PR).

"matching a given expression" is probably ok but it is a bit vague. Maybe "It selects rows where the given expression evaluates to true"? Also, we may want to clarify that the expression must have a return type of boolean.

A list of what?

A list of "field references", where each field reference can either be a string name, integer reference, or fieldref expression.

Updated the documentation and added some more test coverage for this.

Using integers (i.e. a FieldPath) does work out of the box, but given that this position is "relative" to the input of the node (i.e. output of the previous node) and not for the full plan, that's probably not something we want to encourage.

Is there a default? I wonder if it makes sense to default to inner?

I don't have a strong opinion about a default. I think it's fine for a low level interface to require to specify it. On the C++ side there is one simplified constructor signature that defaults to inner, the others are explicit.

This seems a bit sparse. Maybe...

Make a node which joins two inputs together based on common keys. Each row from the left input will be matched with rows from the right input where the keys are equal. Null keys do not match other null keys. If any key is null then that row will not match. Which columns are output, and the behavior of rows that do not match, will depend on the join type.

inner: for each row in the left input there will be one output row per matching row in the right input. If there are no matching rows in the right input then there will be no output row for that input row. The output columns will be the left_keys, followed by the right_keys (these will be identical to the left_keys), followed by the left_output, followed by the right_output.

left outer: behaves the same as an inner join. However, if there are no matching rows in the right input then there will be one output row that contains null for each column in right_keys and right_output. A row in the right input, that has no matching row in the left input, will not be output. The output columns will be identical to an inner join.

right outer: for each row in the right input there will be one output row per matching row in the left input. If there are no matching rows in the left input then there will be one output row containing null in left_keys and left_output. A row in the left input, that has no matching row in the right input, will not be output. The output columns will be identical to an inner join.

full outer: behaves the same as a left outer join. However, if there is a row in the right input, that has no matching row in the left input, then there will be one output row containing null for each column in left_keys and left_output. The output columns will be identical to an inner join.

left semi: for each row in the left input there will be exactly one output row, if and only if, there is a matching row in the right input. The output columns will be the left_keys followed by the left_output.

right semi: for each row in the right input there will be exactly one output row, if and only if, there is a matching row in the left input. The output columns will be the right_keys followed by the right_output.

left anti: for each row in the left input there will be exactly one output row, if and only if, there is not a matching row in the right input. The output will be the same as left semi.

right anti: for each row in the right input there will be exactly one output row, if and only if, there is not a matching row in the left input. The output will be the same as right semi.

Given that this suggestion is a larger addition, going to leave that for a separate PR.

I think users may be confused by this since it might not be clear to them why a "factory name" is a necessary concept at all.

And the factory name is needed for the registry? (so that you can register your own custom nodes)

Do you suggest using a different name here? (eg just "name" instead of "factory name") Or to explain it better what this "factory name" is?

Yes, the factory name is currently needed for the registry. Either you need to explain what it is and explain, in each of the options, what factory name they should use or you need to make the name a part of the options class and remove it from here.

Otherwise, a user might not (for example) know to use "aggregate" when creating AggregateNodeOptions

We could indeed make the factory name unnecessary in the API, if we attach a factory name to the Python option classes. But I think I might opt to stay closer to the C++ API here to require a factory name. But indeed will mention in each option class docstring the name of its factory.

I added some basic explanation. We can always still improve that, or reconsider if the factory name is needed here in python in follow-up PRs.

Do users care about the existence of a sink node? I suppose they might if they are creating custom nodes and aware of such things.

Given that we will have multiple possible sinks at some point, it might be useful to be aware of the general pattern (and also for people that might read the C++ user guide alongside this, I think it is OK to be explicit here)

Uncomment? (I don't remember if I asked about this last time, sorry)

Will just remove for now, will do the Datasets scan/write node option classes in a follow-up PR.

In retrospect, I think immutable is better. In python you can do MyOptions(x=7, y=3). So you don't really need the ability to do:

MyOptions options;
options.x = 7;
options.y = 3;

Do these need to be lists of field references? Or can they be lists of strings?

Yes, they can be any form of field "reference", so either a field_ref or a string (or int). Will clarify in the docstring.

Yes, the factory name is currently needed for the registry. Either you need to explain what it is and explain, in each of the options, what factory name they should use or you need to make the name a part of the options class and remove it from here.

Otherwise, a user might not (for example) know to use "aggregate" when creating AggregateNodeOptions