Allow computed selects that are not orderd by. #13
Conversation
- currently fob throws errors when joined and an expression is used in the select. - Note to select and order by an expression, you still need to use a Ecto fragment
pmonson711
requested review from
the-mikedavis,
electricshaman,
HsuJay,
tonyvanriet and
bjornrud
lib/fob/ordering.ex
Outdated
| Map.new(merges, fn {toplevel_name, | ||
| {{:., _, [{:&, _, [table]}, name_in_table]}, _, _}} -> | ||
| {{table, name_in_table}, toplevel_name} | ||
| Map.new(merges, fn |
There was a problem hiding this comment.
Love this way of creating a new map
There was a problem hiding this comment.
Not a very high value review, so take this with a grain of 🧂, since am unfamiliar.
Appreciate the added explicit example. I do have some worries how this might blow up if there are greedy operations combined with Haste, but that's a Haste problem, right?
Apologies for all the questions and meh low value semantic review stuff.
test/support/left_join_schemas.ex
Outdated
| @@ -7,6 +7,7 @@ defmodule TrunkSchema do | |||
| schema "trunk_schema" do | |||
| field :child, :integer | |||
| field :child_name, :string, virtual: true | |||
| field :computed, :boolean, virtual: true | |||
There was a problem hiding this comment.
- any reason to not change example schema filename to match schema and module name? I've varied them before, but only in an attempt to serve different context. This is an self-contained example right? If so, lock-step makes sense, rite? Apologies if missing something, which is likely here.
| field :computed, :boolean, virtual: true | |
| field :computed?, :boolean, virtual: true |
Because as I read this, a property of the trunk schema is whether or not the schema-schema is itself computed. The trailing ? convention helps readers understand this without knowing the schema-schema's field values for :computed. I promise I won't take this further to Hungarian notation.
That or :example_computed_virtual_field if the field itself is computed. Soz, can envision you saying both the schema and the column are computed, and the schema is computed only because the column (ah, i mean field since no db) is computed. Naming things is hard.
There was a problem hiding this comment.
I dig it, adding the ? should help readability.
RE: reused of the schema, I mostly just reused it because it was there and adding a virtual field doesn't require more migrations. Also I didn't want too add other changes like renaming the table to match the module just for a fix.
There was a problem hiding this comment.
for context I actually updated the field type to :integer as it made the reasoning of the test easier.
test/fob/computed_select_test.exs
Outdated
| s in c.schema, | ||
| select: %{ | ||
| id: s.id, | ||
| computed_column: 1 >= 5 |
There was a problem hiding this comment.
🤔 if there's a way to surface the intent of this always failing for demonstration purposes, if I inferred the intent of this one line's expression correctly
There was a problem hiding this comment.
In my real use case this was a select where some field was a well know string. i.e.
has_special_interest?: s.type == "mytype"But in the test I just wanted to show the most basic if failures in the test. Happy to spend more time on it to try and show intent.
| 5 | ||
| ) | ||
|
|
||
| assert {records, cursor} = Cursor.next(cursor) |
There was a problem hiding this comment.
Iunno how I feel about the asserts depending on one another. Nowait, I dun like it.
# pseudosetup
actual_pages = some_recursive_page_counting_function(bounded_depth_call: 10)
# an expectation I expected of a property of the test
assert actual_pages === 4
Obviously, not terribly helpful with the hand waviness of counting pages that might go on forever and still making the test useful.
There was a problem hiding this comment.
agreed, I find I write a lot of tests that I really wish I could just refute_raise, but I'm sure I can find a better allegory for the asserts
I'm trying to understand the concern, but the current approach should work with haste. There is a very odd dependency that if you both select and order by an expression that includes any operators you need to write it as a fragment. This (I believe) just allows selects to be written naturally when there is no order by on the same bit of logic. I'm sure there is a refactor that would unlock all of Ecto to be used between selects, order by and where clauses but I'm scared of how that interacts with Haste and the PageBreak serialization. |
Prolly just runaway expensive stream 💣 🐉 fears of a long stream with lotsa virtual vields, all handwavy and fear-like as fear tends to be for me. Intent of use, and actual use of something low-level enough as this lib tends to be where pain is so likely nothing to see here. |
the select.
Ecto fragment