Conversation
2. Fill CORE_CONCEPTS.md
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a new SQL AST + compiler and supporting abstractions in example-functional, alongside a set of list/reader refactors (notably consolidating empty list construction via List.nil() and renaming FunctionFrom → Reader).
Changes:
- Add a SQL DSL (
SqlAst,Expr) and string compiler (SqlAstCompiler,ExprCompiler) plusQuery/RowDecoderand an@Alternativeinstance (QueryFunctor). - Add infinite-list interleaving via
Mergedand update zip/lists utilities to useList.nil()consistently. - Rename the function/reader monad implementation to
Readerand update usages/tests/docs.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| sonar-project.properties | Updates SonarQube project key/org settings. |
| functional-definitions/functional-compiler/src/test/java/com/dan323/functional/util/monad/MonadJoin.java | Removes unused import. |
| example-functional/src/test/java/com/dan323/functional/StackActionsTest.java | Updates tests to use List.nil() for empty stacks. |
| example-functional/src/test/java/com/dan323/functional/SqlAstCompilerTest.java | Adds test coverage for SQL AST compilation behavior. |
| example-functional/src/test/java/com/dan323/functional/ReaderTest.java | Updates tests to target Reader instead of FunctionFrom. |
| example-functional/src/test/java/com/dan323/functional/ParserTest.java | Updates an empty-list expectation to List.nil(). |
| example-functional/src/test/java/com/dan323/functional/ListZipperTest.java | Uses List.nil() for empty zipper construction. |
| example-functional/src/test/java/com/dan323/functional/ListTest.java | Uses List.nil() in length assertion. |
| example-functional/src/test/java/com/dan323/functional/FiniteListTest.java | Adjusts tests to use List.nil() with functional ops. |
| example-functional/src/test/java/com/dan323/algebraic/FiniteListMonoidTest.java | Adjusts unit expectation to List.nil(). |
| example-functional/src/main/java/module-info.java | Adds requires java.sql; for JDBC types used by SQL decoder. |
| example-functional/src/main/java/com/dan323/functional/data/state/StackActions.java | Switches FunctionFrom usage to Reader and uses List.nil() for reset/print. |
| example-functional/src/main/java/com/dan323/functional/data/sql/SqlAstCompiler.java | New SQL AST → SQL string compiler with folding/flattening logic. |
| example-functional/src/main/java/com/dan323/functional/data/sql/SqlAst.java | New sealed SQL AST node definitions. |
| example-functional/src/main/java/com/dan323/functional/data/sql/RowDecoder.java | New JDBC ResultSet row decoding abstraction. |
| example-functional/src/main/java/com/dan323/functional/data/sql/QueryFunctor.java | New @Alternative implementation for SQL queries. |
| example-functional/src/main/java/com/dan323/functional/data/sql/Query.java | New record tying SQL AST to a RowDecoder. |
| example-functional/src/main/java/com/dan323/functional/data/sql/ExprCompiler.java | New expression-to-SQL compiler. |
| example-functional/src/main/java/com/dan323/functional/data/sql/Expr.java | New sealed expression AST. |
| example-functional/src/main/java/com/dan323/functional/data/sql/DbError.java | New error ADT for SQL/decoding. |
| example-functional/src/main/java/com/dan323/functional/data/sql/Config.java | New example Reader usage building a JDBC URL. |
| example-functional/src/main/java/com/dan323/functional/data/parser/ParserApplicative.java | Uses List.nil() for many base case. |
| example-functional/src/main/java/com/dan323/functional/data/list/zipper/ListZipperFunctor.java | Uses List.nil() for empty right-side mapping. |
| example-functional/src/main/java/com/dan323/functional/data/list/zipper/ListZipper.java | Initializes left side with List.nil(). |
| example-functional/src/main/java/com/dan323/functional/data/list/ZipApplicative.java | Refines liftA2 handling (incl. Merged support) and switches nil usage. |
| example-functional/src/main/java/com/dan323/functional/data/list/Merged.java | New infinite list that interleaves/merges two infinite lists. |
| example-functional/src/main/java/com/dan323/functional/data/list/ListUtils.java | Refactors concat/reverse to support List and List.nil(). |
| example-functional/src/main/java/com/dan323/functional/data/list/List.java | Adds interleave and broadens cycle to accept List. |
| example-functional/src/main/java/com/dan323/functional/data/list/InfiniteList.java | Updates permitted subclasses to include Merged. |
| example-functional/src/main/java/com/dan323/functional/data/list/FiniteListFunctional.java | Uses List.nil() and adapts concat calls. |
| example-functional/src/main/java/com/dan323/functional/data/list/FiniteList.java | Removes FiniteList.nil() in favor of List.nil() and updates callers. |
| example-functional/src/main/java/com/dan323/functional/data/list/Cycle.java | Adjusts empty-check and concat cast after concat refactor. |
| example-functional/src/main/java/com/dan323/functional/data/function/Reader.java | Renames FunctionFrom to Reader and updates singleton plumbing. |
| example-functional/src/main/java/com/dan323/functional/data/continuation/ContinuationMonad.java | Makes getInstance() return a singleton instance. |
| docs/PARSER.md | Updates examples to use List.nil(). |
| docs/FUNCTIONAL_STRUCTURES.md | Updates examples to use List.nil(). |
| docs/EXAMPLES.md | Updates examples to use List.nil(). |
| CLAUDE.md | Adds repository build/test guidance and architecture notes. |
Comments suppressed due to low confidence (1)
example-functional/src/test/java/com/dan323/functional/ReaderTest.java:12
- Test method names still reference the old
FunctionFromtype (e.g.functionFromFunctor) even though the class under test is nowReader. Renaming the test methods to matchReaderwill keep intent clear and improve searchability.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return c.tableAlias() + "." + c.name(); | ||
| } else if (expr instanceof Expr.Literal<?> l) { | ||
| if (l.value() instanceof String s) { | ||
| return "'" + s + "'"; |
There was a problem hiding this comment.
String literals are emitted by simple quoting without escaping embedded single quotes. This produces invalid SQL for values containing ' and can enable SQL injection if any literals originate from user input. At minimum escape single quotes (e.g. replace ' with ''), or preferably compile literals to parameter placeholders and collect parameters separately.
| return "'" + s + "'"; | |
| String escaped = s.replace("'", "''"); | |
| return "'" + escaped + "'"; |
| return new Query<>(new SqlAst.Empty(), RowDecoder.empty()); | ||
| } | ||
|
|
||
| public static <A> Query<A> disjunction(Query<A> first, Query<A> snd) { |
There was a problem hiding this comment.
disjunction unions the SQL ASTs but always keeps first.decoder(). If snd was built with a different decoder (e.g. via map), the union can decode rows incorrectly or fail at runtime. Consider enforcing decoder compatibility (e.g. require equality / same instance) or defining a safe combined decoder strategy.
| public static <A> Query<A> disjunction(Query<A> first, Query<A> snd) { | |
| public static <A> Query<A> disjunction(Query<A> first, Query<A> snd) { | |
| if (first.decoder() != snd.decoder()) { | |
| throw new IllegalArgumentException("Cannot create disjunction of queries with different decoders"); | |
| } |
| public static <A> FiniteList<A> disjunction(FiniteList<A> op1, FiniteList<A> op2) { | ||
| return ListUtils.concat(op1, op2); | ||
| return (FiniteList<A>) ListUtils.concat(op1, op2); | ||
| } |
There was a problem hiding this comment.
disjunction returns (FiniteList<A>) ListUtils.concat(op1, op2), which is an unchecked downcast from List to FiniteList. It would be safer to have ListUtils.concat return FiniteList when both inputs are finite (or add a dedicated FiniteList concat) so callers don’t need casts.
| public static <A, B> FiniteList<B> flatMap(Function<A, FiniteList<B>> f, FiniteList<A> base) { | ||
| return base.head().maybe(h -> concat(f.apply(h), flatMap(f, base.tail())), FiniteList.nil()); | ||
| return base.head().maybe(h -> (FiniteList<B>) concat(f.apply(h), flatMap(f, base.tail())), List.nil()); | ||
| } |
There was a problem hiding this comment.
flatMap now casts the result of concat(...) back to FiniteList<B>. This is another unchecked cast stemming from concat returning List. Consider tightening concat’s return type for finite inputs (or adding a finite-specific overload) to keep FiniteListFunctional type-safe.
| public record Config(String host, int port, String db) { | ||
|
|
||
| static Function<Config,String> getPath = Reader.<Config>getInstance().liftA2( | ||
| (url, db) -> url + "/" + db, | ||
| Reader.<Config>getInstance() | ||
| .liftA2((host, port) -> "jdbc:postgresql://" + host + ":" + port, Config::host, Config::port), | ||
| Config::db); |
There was a problem hiding this comment.
getPath is declared as a mutable, package-private static field. If this is intended as a stable API constant, make it public static final (or expose it as a method) to prevent accidental reassignment and clarify intended usage.
| private static Expr.Column<Number> col(String alias, String name) { | ||
| return new Expr.Column<>(alias, name, Number.class); | ||
| } | ||
|
|
||
| private static Expr.Column<Integer> colInt(String alias, String name) { | ||
| return new Expr.Column<>(alias, name, Integer.class); | ||
| } |
There was a problem hiding this comment.
The tests (and helpers) build Expr.Column values with a table alias (e.g. "o"/"t"), but SqlAst.Table only carries a table name and SqlAstCompiler never introduces aliases in the rendered FROM/JOIN. This locks in generated SQL like FROM orders WHERE o.amount ... where o is undefined, which won’t execute in SQL engines. Consider extending SqlAst.Table to include an alias (or treating tableAlias as the actual table name) and updating the helpers accordingly so generated SQL declares and consistently uses aliases.
| module functional.data { | ||
| requires functional.annotations; | ||
| requires functional.compiler; | ||
| requires java.sql; |
There was a problem hiding this comment.
New public SQL types were added under com.dan323.functional.data.sql, but the module descriptor does not export that package. If these APIs are intended for external use (and tests import them), add exports com.dan323.functional.data.sql; to avoid JPMS accessibility issues for downstream modules.
| public static <A> FiniteList<A> reverse(FiniteList<A> lst) { | ||
| return lst.head().maybe(h -> concat(reverse(lst.tail()), FiniteList.cons(h, FiniteList.nil())), FiniteList.nil()); | ||
| return lst.head().maybe(h ->(FiniteList<A>) concat(reverse(lst.tail()), FiniteList.cons(h, List.nil())), List.nil()); | ||
| } |
There was a problem hiding this comment.
reverse relies on a forced cast from List to FiniteList. This is only safe if concat(...) is guaranteed to return a FiniteList for finite inputs, but the type system can’t enforce that and a future change could introduce a ClassCastException. Prefer a FiniteList-typed concat overload (e.g. concat(FiniteList, FiniteList)) so reverse stays type-safe.
| // A fragment can be used directly as a JOIN source only when it has no WHERE | ||
| // and no custom projection — otherwise it must be wrapped as a subquery. | ||
| private static String asJoinSource(Fragment fragment) { | ||
| if (fragment instanceof Fragment.Query q && q.where() == null && "*".equals(q.select())) { | ||
| return q.from(); | ||
| } | ||
| return "(" + render(fragment) + ")"; | ||
| } |
There was a problem hiding this comment.
Subqueries/UNION fragments used as FROM/JOIN sources are wrapped in parentheses but not given a derived-table alias. In PostgreSQL (and many other DBs), FROM (SELECT ...) / FROM ((SELECT ...) UNION ...) requires AS <alias>, and without it column references like t.x are invalid. Add aliasing when wrapping fragments (and ensure expressions reference that alias).
|


No description provided.