[DEV 4622] Add support for now in expressions.

[nonibansal]

Important

Add support for now() in expressions for time-based calculations, updating expression handling, serialization, and testing.

• Behavior:
  • Add now() function in expr.py for current time in expressions.
  • Update Expr class to handle Now expressions.
  • Modify QueryEngine to include NOW_COL_NAME in expression evaluation.
• Testing:
  • Add test_now() in test_expr.py to validate now() functionality.
  • Add integration test test_now() in test_expr.py to verify end-to-end behavior.
• Serialization:
  • Update ExprSerializer in serializer.py to serialize Now expressions.
  • Update visitor.py to handle Now expressions in ExprPrinter and FetchReferences.
• Misc:
  • Update CHANGELOG.md for version 1.5.33.
  • Bump version in pyproject.toml to 1.5.33.

^{This description was created by for 880f943. It will automatically update as commits are pushed.}

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to 92016f0 in 36 seconds

More details

• Looked at 659 lines of code in 11 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. fennel/client_tests/test_expr.py:91

• Draft comment:
  Avoid redefining 'now' as it shadows the imported 'now' function. Consider using a different variable name for clarity.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR introduces a new feature to support 'now' in expressions. The implementation seems correct, but there is a minor issue with the use of the 'now' function in the test cases. The 'now' function is being redefined in the test case, which can lead to confusion. It would be better to use a different variable name for the current time in the test case to avoid shadowing the 'now' function.

Workflow ID: wflow_oJZyp2zk9oWErDxr

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 354a9d3 in 42 seconds

More details

• Looked at 659 lines of code in 11 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. fennel/expr/visitor.py:356

• Draft comment:

        return "now()"

• Reason this comment was not posted:
  Confidence changes required: 50%
  The visitNow method in ExprPrinter should return now() instead of NOW() to maintain consistency with the rest of the codebase where now() is used.

Workflow ID: wflow_I0RQEvGy047NEWix

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on c21f77b in 41 seconds

More details

• Looked at 665 lines of code in 11 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. fennel/expr/visitor.py:116

• Draft comment:
  The type hint for when_then_pairs should be List[Tuple[When, Then]] instead of List[When, Then].
• Reason this comment was not posted:
  Confidence changes required: 10%
  The visitWhen method in ExprPrinter and FetchReferences classes uses a list of tuples when_then_pairs to store pairs of When and Then expressions. However, the type hint for when_then_pairs is incorrect. It should be List[Tuple[When, Then]] instead of List[When, Then]. This is a minor issue but should be corrected for clarity and correctness.

2. fennel/expr/visitor.py:185

• Draft comment:
  The type hint for when_then_pairs should be List[Tuple[When, Then]] instead of List[When, Then].
• Reason this comment was not posted:
  Confidence changes required: 10%
  The visitWhen method in ExprPrinter and FetchReferences classes uses a list of tuples when_then_pairs to store pairs of When and Then expressions. However, the type hint for when_then_pairs is incorrect. It should be List[Tuple[When, Then]] instead of List[When, Then]. This is a minor issue but should be corrected for clarity and correctness.

Workflow ID: wflow_oylS6XKlfx5OyeQV

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 1070ca5 in 33 seconds

More details

• Looked at 526 lines of code in 7 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. fennel/expr/expr.py:32

• Draft comment:
  The ExprContext class is introduced but not fully utilized. Consider extending it to handle more context-related information in the future for better scalability.
• Reason this comment was not posted:
  Confidence changes required: 20%
  The ExprContext class is introduced but not fully utilized. It is only used to pass the now_col_name, but it could be extended to handle more context-related information in the future. This is a good design choice for scalability.

2. fennel/expr/expr.py:369

• Draft comment:
  The context parameter in the eval function is a good design choice for extensibility. Consider documenting its usage for clarity.
• Reason this comment was not posted:
  Confidence changes required: 20%
  The eval function in the Expr class now takes an optional context parameter, which is a good design choice for future extensibility. However, the current implementation only uses it for the now_col_name. Consider documenting this parameter for clarity.

3. fennel/testing/query_engine.py:381

• Draft comment:
  The use of ExprContext to pass now_col_name is a good practice. Consider adding documentation for this usage for future reference.
• Reason this comment was not posted:
  Confidence changes required: 20%
  The ExprContext is used in the _compute_expr_extractor method to pass the now_col_name. This is a good use of the context object, but it might be beneficial to document this usage for future reference.

Workflow ID: wflow_CJZWWuPaglvirK3g

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[nikhilgarg28]

Could you please also add this to docs?

[ellipsis-dev]

👍 Looks good to me! Incremental review on e652502 in 34 seconds

More details

• Looked at 808 lines of code in 11 files
• Skipped 1 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. fennel/expr/visitor.py:355

• Draft comment:
  The visitNow method is correctly implemented to handle Now expressions in ExprPrinter. No changes needed.
• Reason this comment was not posted:
  Confidence changes required: 0%
  The visitNow method in ExprPrinter and FetchReferences classes in fennel/expr/visitor.py are correctly implemented to handle Now expressions. No issues found here.

2. fennel/expr/visitor.py:459

• Draft comment:
  The visitNow method is correctly implemented to handle Now expressions in FetchReferences. No changes needed.
• Reason this comment was not posted:
  Confidence changes required: 0%
  The visitNow method in FetchReferences class in fennel/expr/visitor.py is correctly implemented to handle Now expressions. No issues found here.

Workflow ID: wflow_DNkEOLWeKTeMPAXd

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 236909b in 16 seconds

More details

• Looked at 75 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. docs/pages/api-reference/expressions/now.md:8

• Draft comment:
  The description is unclear and contains a grammatical error. Consider rephrasing to: "Function to obtain the current timestamp as a column, similar to current_timestamp() in Polars or Spark."
• Reason this comment was not posted:
  Confidence changes required: 50%
  The description of the now function is unclear and contains a grammatical error. It should be improved for clarity.

Workflow ID: wflow_oIUsPvhea669oWjg

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 093b132 in 14 seconds

More details

• Looked at 75 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. docs/pages/api-reference/expressions/now.md:8

• Draft comment:
  The description is unclear and contains a grammatical error. Consider revising to: "Function to obtain the current timestamp as a column, similar to current_timestamp() in Polars or Spark."
• Reason this comment was not posted:
  Confidence changes required: 50%
  The description of the now function in the documentation is unclear and contains a grammatical error. It should be revised for clarity.

Workflow ID: wflow_h2NLh4ikdQpoxqCU

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 6f20f93 in 18 seconds

More details

• Looked at 31 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. fennel/expr/expr.py:366

• Draft comment:
  The now() function is defined but not integrated into the Expr class for expression handling. Consider adding a method in the Expr class to handle Now expressions.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_2EUDaJvbEe5A3s92

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 9843d79 in 25 seconds

More details

• Looked at 15 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. docs/api.yml:99

• Draft comment:
  The now expression was moved incorrectly. It should be placed in the correct alphabetical order within the list.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment claims that the 'now' expression is out of order, but upon reviewing the list, it seems to be in the correct alphabetical position. The list is ordered alphabetically, and 'now' fits correctly between 'not' and 'typeof'. Therefore, the comment seems to be incorrect.
  I might be missing some context about the ordering rules for this specific file, but based on standard alphabetical ordering, the placement seems correct.
  The alphabetical order is a standard convention, and without additional context suggesting a different ordering rule, the current placement appears correct.
  The comment is incorrect as the 'now' expression is in the correct alphabetical order. The comment should be deleted.

Workflow ID: wflow_cFOAfMB35kfuTX8a

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[nikhilgarg28]

Approving tentatively though plz make the couple small changes suggested.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 880f943 in 34 seconds

More details

• Looked at 942 lines of code in 16 files
• Skipped 1 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. fennel/datasets/test_invalid_dataset.py:298

• Draft comment:
  The error message has been changed to lowercase. Ensure consistency with other error messages in the codebase.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR introduces a new now() function in expressions, which is a significant addition. The implementation seems to be consistent across the codebase, including the necessary changes in the expression handling, serialization, and testing. However, there are a few areas that need attention. The error messages in test_invalid_dataset.py and test_invalid_expr.py have been changed to lowercase, which might not be consistent with the rest of the codebase. Additionally, the EvalContext class is introduced but not fully utilized in all relevant parts of the code. The NOW_COL_NAME is defined in multiple places, which could lead to inconsistencies. It would be better to centralize this definition. Overall, the PR is well-structured, but these minor issues should be addressed.

2. fennel/expr/test_invalid_expr.py:30

• Draft comment:
  The error message has been changed to lowercase. Ensure consistency with other error messages in the codebase.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR introduces a new now() function in expressions, which is a significant addition. The implementation seems to be consistent across the codebase, including the necessary changes in the expression handling, serialization, and testing. However, there are a few areas that need attention. The error messages in test_invalid_dataset.py and test_invalid_expr.py have been changed to lowercase, which might not be consistent with the rest of the codebase. Additionally, the EvalContext class is introduced but not fully utilized in all relevant parts of the code. The NOW_COL_NAME is defined in multiple places, which could lead to inconsistencies. It would be better to centralize this definition. Overall, the PR is well-structured, but these minor issues should be addressed.

3. fennel/testing/query_engine.py:26

• Draft comment:
  Consider centralizing the definition of NOW_COL_NAME to avoid inconsistencies.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The NOW_COL_NAME is defined in multiple places, which could lead to inconsistencies. It would be better to centralize this definition to avoid potential errors in the future.

Workflow ID: wflow_yTbQf9qcPoRDoQh7

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.