Implement power operator #651

progress0407 · 2024-02-21T11:36:46Z

Motivation

This PR on the subject

Modifications

Write power() in Jpql, Expressions
Create a JpqlPowerSerializer
Write the respective test code

Commit Convention Rule

Please commit your modification based by conventional commit:

Commit type	Description
feat	New Feature
fix	Fix bug
docs	Documentation only changed
ci	Change CI configuration
refactor	Not a bug fix or add feature, just refactoring code
test	Add Test case or fix wrong test case
style	Only change the code style(ex. white-space, formatting)
chore	It refers to minor tasks such as library version upgrade, typo correction, etc.

If you want to add some more commit type please describe it on the Pull Request

Result

Now supports POWER.

Closes

Proposal: Adding Power Operator Support to Expressions #647

codecov-commenter · 2024-02-21T12:05:22Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (37a3bac) 67.05% compared to head (5369802) 67.15%.

Files	Patch %	Lines
...render/jpql/serializer/impl/JpqlPowerSerializer.kt	88.88%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #651      +/-   ##
===========================================
+ Coverage    67.05%   67.15%   +0.10%     
===========================================
  Files          479      481       +2     
  Lines         4863     4881      +18     
  Branches       277      277              
===========================================
+ Hits          3261     3278      +17     
- Misses        1545     1546       +1     
  Partials        57       57

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

progress0407 · 2024-02-21T12:14:07Z

We see that there is missing test code.
I'll fill it in and post it.

progress0407 · 2024-02-22T02:08:58Z

um...

I don't know why that one line of missed code is there.

Because as you can see, when I run the test for that class, it definitely captures the debugger.

I guess I'll have to find out what the magic is...

cj848 · 2024-02-22T03:14:28Z

dsl/jpql/src/main/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/Jpql.kt

+     * Creates an expression that represents the mod of values.
+     */
+    @SinceJdsl("3.4.0")
+    fun <T : Any> mod(expr: KProperty1<T, Int>, value: Int): Expression<Int> {


It appears that the mod has not been implemented, but the commit appears to have been distorted.

Oops, I had them in the order they appear in the official documentation, which seems to have caused some confusion. I've now fixed them back to their original position.

waahhh · 2024-02-22T06:11:06Z

docs/en/jpql-with-kotlin-jdsl/expressions.md

@@ -247,6 +249,8 @@ ln(path(Book::price))

 mod(path(Employee::age), 3)

+power(path(Book::verticalLength), 2)


Is the Book::verticalLength property new?
It would be better to use an existing property (e.g. Employee::age)

Oh, right, I thought about adding that field, but it's not currently there.
I know someone added an age field recently haha I'll replace it with that.

waahhh · 2024-02-22T06:21:59Z

dsl/jpql/src/main/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/Jpql.kt

+     * Create expression that calculates the powering of a numeric [base] to a specified [exponent].
+     */
+    @SinceJdsl("3.4.0")
+    fun <T : Number> power(base: Expressionable<T>, exponent: Int): Expression<T> {


Could you please write a method based on the specification in jakarta specification?

Wow, I love doing reviews based on such solid knowledge. lol Thank you :)

I feel like this allows me to get up close and personal with the Hibernate spec.

I would recommend following the jakarta spec unless you have a special use case. This is because if Hibernate and EclipseLink have different specifications, you may not be able to decide which one to follow.

Oh... Thank you.
I gained a lot of knowledge while implementing this power.
I also understand now why the package was changed from javax to jakrta.

References

https://medium.com/@nagarjunmallesh/java-persistence-api-vs-jakarta-persistence-layer-a-comparative-study-with-code-implementation-4462b1abcfd1

https://www.samsungsds.com/kr/insights/java_jakarta.html

waahhh · 2024-02-22T06:33:07Z

dsl/jpql/src/test/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/expression/PowerDslTest.kt

+    private val baseExpression = Paths.path(Employee::age)
+    private val exponentExpression = Expressions.value(2)
+    private val exponentPrimitive = 2


This seems to be done to make the semantics of the variables clearer, but it would be nice to have a consistent style with other existing test code (e.g. ModDslTest.kt)

Suggested change

private val baseExpression = Paths.path(Employee::age)

private val exponentExpression = Expressions.value(2)

private val exponentPrimitive = 2

private val intExpression1 = Paths.path(Employee::age)

private val intExpression2 = Expressions.value(2)

private val int1 = 2

Consistency rather than semantics, I see :)

waahhh · 2024-02-22T06:37:03Z

render/jpql/src/main/kotlin/com/linecorp/kotlinjdsl/render/jpql/JpqlRenderContext.kt

@@ -370,6 +371,7 @@ private class DefaultModule : JpqlRenderModule {
            JpqlPredicateParenthesesSerializer(),
            JpqlRoundSerializer(),
            JpqlSelectQuerySerializer(),
+            JpqlPowerSerializer(),


Could you please move the position of JpqlPowerSerializer() below JpqlPlusSerializer()?

You mean alphabetical order! Got it :)

waahhh · 2024-02-22T06:43:17Z

um...

I don't know why that one line of missed code is there.

Because as you can see, when I run the test for that class, it definitely captures the debugger.

I guess I'll have to find out what the magic is...

You don't need to worry about this missing test code issue.

waahhh · 2024-02-22T06:53:11Z

dsl/jpql/src/main/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/Jpql.kt

+     * Create expression that calculates the powering of a numeric [base] to a specified [exponent].
+     */
+    @SinceJdsl("3.4.0")
+    fun <T : Any, V : Number> power(base: KProperty1<T, @Exact V>, exponent: Expression<Int>): Expression<V> {


When used as a parameter to a dsl method, it is more appropriate to write it with the Expressionable<> type instead of the Expression<> type

e.g.

Suggested change

fun <T : Any, V : Number> power(base: KProperty1<T, @Exact V>, exponent: Expression<Int>): Expression<V> {

fun <T : Any, V : Number> power(base: KProperty1<T, @Exact V>, exponent: Expressionable<Int>): Expression<V> {

Is it to accept a wider range of material types?? reflected ! :)

I also see this in other code that I haven't checked. I'll fix it in one shot later.

Thanks for checking it out.
I'm actually pretty much done with it, but I've had a little bit of a break, so I haven't had a chance to do a final review and PR.
I'll probably PR it really soon.
Thanks.

progress0407 · 2024-02-22T07:11:38Z

Thank you so much to everyone who reviewed my code! (I'm touched)
However, due to my work, I won't be able to implement them right away.
I've read everything you said, and I'll apply what I can to the PR after committing!
Thank you everyone! (especially waah)

shouwn · 2024-02-23T10:12:56Z

dsl/jpql/src/main/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/Jpql.kt

+    }
+
+    /**
+     * Create expression that calculates the powering of a numeric [base] to a specified [exponent].


How about writing something like this?

Suggested change

* Create expression that calculates the powering of a numeric [base] to a specified [exponent].

* Create an expression that represents the power of [base] and [exponent].

thank you. The sentence below looks easier to read.

shouwn · 2024-02-23T10:14:43Z

dsl/jpql/src/main/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/Jpql.kt

+     * Create expression that calculates the powering of a numeric [base] to a specified [exponent].
+     */
+    @SinceJdsl("3.4.0")
+    fun <T : Number> power(base: Expressionable<T>, exponent: Int): Expression<T> {


I would recommend following the jakarta spec unless you have a special use case. This is because if Hibernate and EclipseLink have different specifications, you may not be able to decide which one to follow.

shouwn · 2024-02-23T10:16:06Z

dsl/jpql/src/main/kotlin/com/linecorp/kotlinjdsl/dsl/jpql/Jpql.kt

+     * Create expression that calculates the powering of a numeric [base] to a specified [exponent].
+     */
+    @SinceJdsl("3.4.0")
+    fun <T : Any, V : Number> power(base: KProperty1<T, @Exact V>, exponent: Expression<Int>): Expression<V> {


I also see this in other code that I haven't checked. I'll fix it in one shot later.

progress0407 · 2024-02-23T17:01:28Z

I've been busy for a while... I'll PR it soon!

shouwn

Thanks for your help. I will make some revisions, such as comments, after I merge it into the develop branch.

progress0407 added 5 commits February 21, 2024 14:58

feat: implement power arithmetic operation

67a4be2

docs: update expressions docs

d7744f1

style: merged from develop and reorder expressions

2384029

test: remove book property and choose employ fixture

9822e0b

style: rewrite comments to conform to conventions

bfb8d48

progress0407 requested review from pickmoment, huisam, cj848 and shouwn as code owners February 21, 2024 11:36

progress0407 closed this Feb 21, 2024

fix: fix poorly implemented serializer code

8b80467

progress0407 reopened this Feb 21, 2024

progress0407 closed this Feb 21, 2024

progress0407 changed the title ~~Implement power operator~~ Implement power operator (to be open) Feb 21, 2024

progress0407 added 2 commits February 22, 2024 10:11

fix: fix poorly implemented jpql code and test

69109b2

Merge branch 'develop' into feat/arith-power

5369802

progress0407 reopened this Feb 22, 2024

progress0407 changed the title ~~Implement power operator (to be open)~~ Implement power operator Feb 22, 2024

cj848 reviewed Feb 22, 2024

View reviewed changes

waahhh reviewed Feb 22, 2024

View reviewed changes

docs: reflect reviews of updating example

1bfb2cf

shouwn reviewed Feb 23, 2024

View reviewed changes

progress0407 added 2 commits March 13, 2024 01:34

refactor: reflect reviews of updating example

5d231b1

Merge branch 'develop' into feat/arith-power

c4a8a6a

progress0407 requested review from cj848, waahhh and shouwn March 12, 2024 17:14

shouwn approved these changes Mar 12, 2024

View reviewed changes

shouwn merged commit 181aa4b into line:develop Mar 12, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement power operator #651

Implement power operator #651

progress0407 commented Feb 21, 2024 •

edited

Loading

codecov-commenter commented Feb 21, 2024 •

edited

Loading

progress0407 commented Feb 21, 2024

progress0407 commented Feb 22, 2024

cj848 Feb 22, 2024

progress0407 Feb 22, 2024

waahhh Feb 22, 2024

progress0407 Feb 22, 2024

waahhh Feb 22, 2024

progress0407 Feb 22, 2024

shouwn Feb 23, 2024

progress0407 Feb 23, 2024

waahhh Feb 22, 2024

progress0407 Feb 22, 2024

waahhh Feb 22, 2024

progress0407 Feb 22, 2024

waahhh commented Feb 22, 2024

waahhh Feb 22, 2024

progress0407 Feb 22, 2024

shouwn Feb 23, 2024

progress0407 Feb 23, 2024

progress0407 commented Feb 22, 2024

shouwn Feb 23, 2024

progress0407 Feb 23, 2024

shouwn Feb 23, 2024

shouwn Feb 23, 2024

progress0407 commented Feb 23, 2024

shouwn left a comment

		@@ -247,6 +249,8 @@ ln(path(Book::price))

		mod(path(Employee::age), 3)

		power(path(Book::verticalLength), 2)

	fun <T : Any, V : Number> power(base: KProperty1<T, @Exact V>, exponent: Expression<Int>): Expression<V> {
	fun <T : Any, V : Number> power(base: KProperty1<T, @Exact V>, exponent: Expressionable<Int>): Expression<V> {

	* Create expression that calculates the powering of a numeric [base] to a specified [exponent].
	* Create an expression that represents the power of [base] and [exponent].

Implement power operator #651

Implement power operator #651

Conversation

progress0407 commented Feb 21, 2024 • edited Loading

Motivation

Modifications

Commit Convention Rule

Result

Closes

codecov-commenter commented Feb 21, 2024 • edited Loading

Codecov Report

progress0407 commented Feb 21, 2024

progress0407 commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waahhh commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

progress0407 commented Feb 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

progress0407 commented Feb 23, 2024

shouwn left a comment

Choose a reason for hiding this comment

progress0407 commented Feb 21, 2024 •

edited

Loading

codecov-commenter commented Feb 21, 2024 •

edited

Loading